Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duplicate props with @next/codemod new-link #41925

Closed
1 task done
andrewmartin opened this issue Oct 27, 2022 · 7 comments
Closed
1 task done

Duplicate props with @next/codemod new-link #41925

andrewmartin opened this issue Oct 27, 2022 · 7 comments
Labels
good first issue Easy to fix issues, good for newcomers

Comments

@andrewmartin
Copy link

Verify canary release

  • I verified that the issue exists in the latest Next.js canary release

Provide environment information

I'm not sure this is super relevant as it's related to the @next/codemod package, but here you go:

    Operating System:
      Platform: darwin
      Arch: x64
      Version: Darwin Kernel Version 22.1.0: Sun Oct  9 20:14:54 PDT 2022; root:xnu-8792.41.9~2/RELEASE_X86_64
    Binaries:
      Node: 14.20.0
      npm: 6.14.17
      Yarn: 1.22.5
      pnpm: N/A
    Relevant packages:
      next: 13.0.0
      eslint-config-next: 12.1.6
      react: 18.2.0
      react-dom: 18.2.0

What browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

Describe the Bug

I think I may have used the <Link /> API incorrectly, so I'm not sure this falls into the path of something that should be handled by the mod, but I ran:

npx @next/codemod new-link .

And it worked, but I had to manually go thru and fix No duplicate props allowed react/jsx-no-duplicate-props errors because all of my links had two href attributes after the mod.

Easy enough to fix, but I thought I'd throw it out there as something maybe the codemod could account for?

<Link href={href} href={href} />

Expected Behavior

I should only see one href attribute.

Link to reproduction

NA

To Reproduce

This should happen on any next repo with a <Link href={href} /><a href={href}></a></Link> element structure.

@andrewmartin andrewmartin added the bug Issue was opened via the bug report template. label Oct 27, 2022
@balazsorban44 balazsorban44 added kind: bug good first issue Easy to fix issues, good for newcomers and removed bug Issue was opened via the bug report template. labels Oct 27, 2022
@Pranav-yadav
Copy link

Pranav-yadav commented Oct 28, 2022

@balazsorban44 I would like to help w/ this one; if it's not assigned to anyone already :)

PS: I think this issue has something to do w/ following lines of code ?

// Direct child elements referenced
const $childrenElements = $link.childElements()
const $childrenWithA = $childrenElements.filter((childPath) => {
return (
j(childPath).find(j.JSXOpeningElement).get('name').get('name')
.value === 'a'
)
})

const props = $childrenWithA.get('attributes').value
const hasProps = props.length > 0
if (hasProps) {
// Add props to <Link>
$link.get('attributes').value.push(...props)
// Remove props from <a>
props.length = 0
}

@ishaqibrahimbot
Copy link
Contributor

Hey @balazsorban44!

I've submitted a PR that fixes this issue. Would be great if you could review it and if satisfactory, let me know of any due diligence I might need to do to get this merged. I'm fairly new to OS contributions and this project as well, so would appreciate any guidance about steps I might've missed. Thanks!

@andrewmartin
Copy link
Author

Added a comment, great addition and I think it'd be cool to see this in a shared helper or util somewhere. Seems like a common use case to avoid spreading and adding duplicate props.

@ishaqibrahimbot
Copy link
Contributor

Yeah seems to be a handy pattern. I'll look into where it would be best to put this and how it would be interfaced with.

ijjk pushed a commit that referenced this issue Oct 30, 2022
…chor to link (#42158)

Fixes issue #41925 by skipping duplicate props. I've tested the changes
locally - the duplicate prop is being skipped successfully. The
new-link.test.js suite is also passing.

<!--
Thanks for opening a PR! Your contribution is much appreciated.
To make sure your PR is handled as smoothly as possible we request that
you follow the checklist sections below.
Choose the right checklist for the change that you're making:
-->

## Bug

- [x] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Errors have a helpful link attached, see `contributing.md`

## Feature

- [ ] Implements an existing feature request or RFC. Make sure the
feature request has been accepted for implementation before opening a
PR.
- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have a helpful link attached, see `contributing.md`

## Documentation / Examples

- [ ] Make sure the linting passes by running `pnpm build && pnpm lint`
- [ ] The "examples guidelines" are followed from [our contributing
doc](https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md)

Co-authored-by: Ishaq Ibrahim <ishaq.ibrahim@convertdigital.com.au>
@koba04
Copy link
Contributor

koba04 commented Apr 20, 2023

This issue has already been fixed at #42158, so this can be closed.

@timneutkens
Copy link
Member

Thanks @koba04!

@github-actions
Copy link
Contributor

github-actions bot commented Jun 3, 2023

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Easy to fix issues, good for newcomers
Projects
None yet
Development

No branches or pull requests

6 participants