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

Improve RSC plugin to provide better errors #42435

Merged
merged 12 commits into from Nov 24, 2022
Merged

Improve RSC plugin to provide better errors #42435

merged 12 commits into from Nov 24, 2022

Conversation

shuding
Copy link
Member

@shuding shuding commented Nov 3, 2022

This PR improves the RSC plugin for SWC to throw an error when the "use client" directive is in the top level, but not before other statements / expressions. For example:

Code:

import 'react'

'use client'

Error:

The "use client" directive must be placed before other expressions. Move it to the top of the file to resolve this issue.

   ,----
 3 | 'use client'
   : ^^^^^^^^^^^^
   `----

Bug

  • 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

@ijjk
Copy link
Member

ijjk commented Nov 3, 2022

Failing test suites

Commit: 7c183f6

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

  • ReactRefreshLogBox app > render error not shown right after syntax error
Expand output

● ReactRefreshLogBox app › render error not shown right after syntax error

expect(received).toMatchSnapshot()

Snapshot name: `ReactRefreshLogBox app render error not shown right after syntax error 1`

- Snapshot  -  9
+ Received  + 11

- index.js (6:18) @ ClassDefault.render
-
-   4 | class ClassDefault extends React.Component {
-   5 |   render() {
- > 6 |     throw new Error('nooo');
-     |          ^
-   7 |     return <h1>Default Export</h1>;
-   8 |   }
-   9 | }
+ ./index.js
+ Error: 
+   x Expected '{', got 'throw'
+    ,----
+  6 | throw new Error('nooo');
+    : ^^^^^
+    `----
+
+ Caused by:
+     0: failed to process input file
+     1: Syntax Error

  327 |     )
  328 |     expect(await session.hasRedbox(true)).toBe(true)
> 329 |     expect(await session.getRedboxSource()).toMatchSnapshot()
      |                                             ^
  330 |
  331 |     await cleanup()
  332 |   })

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

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

Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is incorrectly erroring in Generate PR stats https://github.com/vercel/next.js/actions/runs/3448885518/jobs/5756416759#step:6:348

ijjk
ijjk previously approved these changes Nov 23, 2022
@ijjk ijjk dismissed their stale review November 24, 2022 00:07

skip kodiak

@ijjk ijjk merged commit 60d5c96 into canary Nov 24, 2022
@ijjk ijjk deleted the shu/d046 branch November 24, 2022 01:26
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants