Run next/link codemod for Next.js 13 on examples #41913
Conversation
timneutkens
requested review from
ijjk,
shuding,
styfle,
leerob,
steven-tey and
balazsorban44
as code owners
ijjk
added
examples
| @@ -14,7 +14,7 @@ export default function Index() { | |||
| <ul> | |||
| {data.map((user) => ( | |||
| <li key={user.id}> | |||
| <Link href="/user/[id]" as={`/user/${user.id}`}> | |||
| <Link href="/user/[id]" as={`/user/${user.id}`} legacyBehavior> | |||
There was a problem hiding this comment.
Does this one need to be legacy? Looks like the child is already correct
| @@ -11,30 +11,30 @@ const Header = ({ user, loading }: HeaderProps) => { | |||
| <nav> | |||
| <ul> | |||
| <li> | |||
| <Link href="/"> | |||
| <Link href="/" legacyBehavior> | |||
| <a>Home</a> | |||
There was a problem hiding this comment.
Seems like a bug: i would expect this anchor to be removed
There was a problem hiding this comment.
Hey @styfle!
I was just taking a look around this PR and decided to check this out. The transform not removing the anchor tags in this file and instead adding the legacyBehavior prop is intentional. This is because of the presence of a <style> tag with a jsx attribute in this component which makes removing the anchor tag a bit risky in case the anchor tag has some CSS applied to it inside the <style> tag. See screenshot below and refer to this commit by Tim.
Do you think it would be wise to log a simple warning message in this case? So that anyone running the codemod is alerted about any file(s) that might be treated like this because of the presence of <style jsx>? I can submit a PR with the warning message added. Thanks!
There was a problem hiding this comment.
Do we know why a style tag would affect the link? Seems like emitted HTML should be the same since “Link” becomes “a” with the new behavior.
There was a problem hiding this comment.
This is probably because of scope limitations, i.e. the <style jsx> affects any tags inside the same parent component's scope, but not tags that are returned from React components inside that same parent component.
See this example in the styled-jsx docs for a better explanation:
I suppose this is why the decision was taken to just add the legacyBehavior prop if a <style jsx> tag is found in a file. Users who might have directly styled the anchor tag inside the <style jsx> tag will see those styles disappear in the UI if we don't go the legacyBehavior way.
There was a problem hiding this comment.
There are some edge cases that seemingly haven't fully been considered by the codemod, so we might want to fix that first and add some tests. We could fix #41925 at the same time too
|
@balazsorban44 @timneutkens Lets merge this? Seems like the issues it will fix are greater than the issues it introduces. For example, it should close |
x-ref: #41913 ## 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)
Ensures the examples show
<Link>usage correctly.Bug
fixes #numbercontributing.mdFeature
fixes #numbercontributing.mdDocumentation / Examples
pnpm lint