-
Notifications
You must be signed in to change notification settings - Fork 26k
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
Run next/link codemod for Next.js 13 on examples #41913
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like a bug: i would expect this anchor to be removed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
</Link> | ||
</li> | ||
<li> | ||
<Link href="/about"> | ||
<Link href="/about" legacyBehavior> | ||
<a>About</a> | ||
</Link> | ||
</li> | ||
<li> | ||
<Link href="/advanced/api-profile"> | ||
<Link href="/advanced/api-profile" legacyBehavior> | ||
<a>API rendered profile (advanced)</a> | ||
</Link> | ||
</li> | ||
{!loading && | ||
(user ? ( | ||
<> | ||
<li> | ||
<Link href="/profile"> | ||
<Link href="/profile" legacyBehavior> | ||
<a>Client rendered profile</a> | ||
</Link> | ||
</li> | ||
<li> | ||
<Link href="/advanced/ssr-profile"> | ||
<Link href="/advanced/ssr-profile" legacyBehavior> | ||
<a>Server rendered profile (advanced)</a> | ||
</Link> | ||
</li> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this one need to be legacy? Looks like the child is already correct