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

feat: block compilation on codemod comments and ask to remove #71103

Merged
merged 14 commits into from
Oct 11, 2024

Conversation

huozhi
Copy link
Member

@huozhi huozhi commented Oct 10, 2024

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.

  • If you know the error and think it's safe to bypass now and handle it later
- /* @next-codemod-error codemod tells you something wrong with this line */
+ /* @next-codemod-ignore */
  • If you know you've already addressed the issue, just delete the comment
- /* @next-codemod-error codemod tells you something wrong with this line */
Error sample in the Dev Overlay
./app/page.tsx

Error:   x You have unresolved @next/codemod comment "Manually await this call, if it's a Server Component" that need to be reviewed, please address and remove them to proceed with the build.
  | You can also bypass the build error by replacing "@next-codemod-error" with "@next-codemod-ignore".
    ,-[2:1]
  1 | export default function Page() {
  2 |   // @next-codemod-error Manually await this call, if it's a Server Component
    :  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  3 |   return <p>hello world</p>
  4 | }
    \`----

Why

This will help users have a more complete migration, and avoid accidentally go production with the unmirgated cases unhandled.

Copy link
Member Author

huozhi commented Oct 10, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @huozhi and the rest of your teammates on Graphite Graphite

@huozhi huozhi changed the title Add lint_error_comment.rs feat: block compilation on codemod comments and ask to remove Oct 10, 2024
@ijjk
Copy link
Member

ijjk commented Oct 10, 2024

Failing test suites

Commit: f51fd13

pnpm test-dev test/development/acceptance-app/ReactRefreshLogBox.test.ts

  • ReactRefreshLogBox app default > server component can recover from error thrown in the module
Expand output

● ReactRefreshLogBox app default › server component can recover from error thrown in the module

Expected Redbox but found none

  1102 |
  1103 |       await next.patchFile('index.js', "throw new Error('module error')")
> 1104 |       await session.assertHasRedbox()
       |       ^
  1105 |       await next.patchFile(
  1106 |         'index.js',
  1107 |         'export default function Page() {return <p>hello world</p>}'

  at Object.<anonymous> (development/acceptance-app/ReactRefreshLogBox.test.ts:1104:7)

Read more about building and testing Next.js in contributing.md.

@ijjk ijjk added created-by: Next.js team PRs by the Next.js team. tests Turbopack Related to Turbopack with Next.js. type: next labels Oct 10, 2024
Copy link
Member

@kdy1 kdy1 left a 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

@huozhi huozhi force-pushed the feat/error-on-codemod-comments branch from 4f0b070 to 3155c90 Compare October 11, 2024 13:06
@huozhi huozhi marked this pull request as ready for review October 11, 2024 13:21
@eps1lon
Copy link
Member

eps1lon commented Oct 11, 2024

Do we also support //-style comments?

Copy link
Member

@eps1lon eps1lon left a 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

@ijjk ijjk added the Documentation Related to Next.js' official documentation. label Oct 11, 2024
@huozhi
Copy link
Member Author

huozhi commented Oct 11, 2024

Do we also support //-style comments?

Now we do (after few commits)

@huozhi huozhi requested a review from eps1lon October 11, 2024 15:20
errors/sync-dynamic-apis.mdx Show resolved Hide resolved
errors/sync-dynamic-apis.mdx Outdated Show resolved Hide resolved
errors/sync-dynamic-apis.mdx Outdated Show resolved Hide resolved
errors/sync-dynamic-apis.mdx Outdated Show resolved Hide resolved
@huozhi huozhi requested a review from ztanner October 11, 2024 16:05
Ecmascript file had an error
1 | export default function Page() {
> 2 | // @next-codemod-error remove jsx of next line
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Copy link
Member

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.

Comment on lines +37 to +38
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."
Copy link
Member

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.

@huozhi huozhi merged commit 1b89b4b into canary Oct 11, 2024
100 of 108 checks passed
@huozhi huozhi deleted the feat/error-on-codemod-comments branch October 11, 2024 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
created-by: Next.js team PRs by the Next.js team. Documentation Related to Next.js' official documentation. tests Turbopack Related to Turbopack with Next.js. type: next
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants