-
Notifications
You must be signed in to change notification settings - Fork 27.8k
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
feat: block compilation on codemod comments and ask to remove #71103
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
crates/next-custom-transforms/src/transforms/lint_error_comment.rs
Outdated
Show resolved
Hide resolved
crates/next-custom-transforms/src/transforms/lint_error_comment.rs
Outdated
Show resolved
Hide resolved
Failing test suitesCommit: f51fd13
Expand output● ReactRefreshLogBox app default › server component can recover from error thrown in the module
Read more about building and testing Next.js in contributing.md. |
crates/next-custom-transforms/src/transforms/lint_codemod_comments.rs
Outdated
Show resolved
Hide resolved
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.
Overall it looks good
4f0b070
to
3155c90
Compare
Do we also support |
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.
Super helpful 👍🏻
This should be documented in errors/sync-dynamic-apis.mdx
crates/next-custom-transforms/src/transforms/lint_codemod_comments.rs
Outdated
Show resolved
Hide resolved
Now we do (after few commits) |
crates/next-custom-transforms/src/transforms/lint_codemod_comments.rs
Outdated
Show resolved
Hide resolved
...sforms/__testfixtures__/next-async-request-api-dynamic-apis/async-api-type-cast-02.output.js
Outdated
Show resolved
Hide resolved
Ecmascript file had an error | ||
1 | export default function Page() { | ||
> 2 | // @next-codemod-error remove jsx of next line | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ |
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.
aside: column is off by one here. We also have other tests where we observe this. Just making a mental note for when we revisit the error overlay.
You have an unresolved @next/codemod comment "remove jsx of next line" that needs review. | ||
After review, either remove the comment if you made the necessary changes or replace "@next-codemod-error" with "@next-codemod-ignore" to bypass the build error if no action at this line can be taken." |
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.
for turbopack: I like the error before the sourceframe more.
What
Introduce a SWC validator that will error for specific comments, this is introduced for the comments left by codemod. The erroring logic is the new check after users upgrade to v15 and codemod is applied, which enforces users to address all the codemod comments before dev or build. With the new dynamic async API model, any comment that is not addressed could lead to possible issues in production. Hence this linter could enforce you review the changes and remove the comments left by codemod.
Example
After running next-codemod against your codebase where it has some cases are not able to be migrated by codemod.
Next.js codemod will leave comment
/* @next-codemod-error ... */
for the case that not able to handle, and this will error in dev and build as compilation error. You either delete it or just replace the prefix to@next-codemod-error
, such as/* @next-codemod-ignore */
to bypass the compilation error.- /* @next-codemod-error codemod tells you something wrong with this line */
Error sample in the Dev Overlay
Why
This will help users have a more complete migration, and avoid accidentally go production with the unmirgated cases unhandled.