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
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintignore
Expand Up @@ -29,6 +29,7 @@ test/integration/eslint/**
test/integration/script-loader/**/*
test/development/basic/legacy-decorators/**/*
test/production/emit-decorator-metadata/**/*.js
test/e2e/app-dir/rsc-errors/app/swc/use-client/page.js
test-timings.json
packages/next-swc/crates/**
bench/nested-deps/pages/**
Expand Down
67 changes: 47 additions & 20 deletions packages/next-swc/crates/core/src/react_server_components.rs
Expand Up @@ -84,33 +84,60 @@ impl<C: Comments> ReactServerComponents<C> {
let _ = &module.body.retain(|item| {
match item {
ModuleItem::Stmt(stmt) => {
if !finished_directives {
if !stmt.is_expr() {
// Not an expression.
finished_directives = true;
}
if !stmt.is_expr() {
// Not an expression.
finished_directives = true;
}

match stmt.as_expr() {
Some(expr_stmt) => {
match &*expr_stmt.expr {
Expr::Lit(Lit::Str(Str { value, .. })) => {
if &**value == "use client" {
match stmt.as_expr() {
Some(expr_stmt) => {
match &*expr_stmt.expr {
Expr::Lit(Lit::Str(Str { value, .. })) => {
if &**value == "use client" {
if !finished_directives {
is_client_entry = true;

// Remove the directive.
return false;
} else {
HANDLER.with(|handler| {
handler
.struct_span_err(
expr_stmt.span,
"NEXT_RSC_ERR_CLIENT_DIRECTIVE",
)
.emit()
})
}

// Remove the directive.
return false;
}
_ => {
// Other expression types.
finished_directives = true;
}
// Match `ParenthesisExpression` which is some formartting tools
// usually do: ('use client'). In these case we need to throw
// an exception because they are not valid directives.
Expr::Paren(ParenExpr { expr, .. }) => {
finished_directives = true;
if let Expr::Lit(Lit::Str(Str { value, .. })) = &**expr {
if &**value == "use client" {
HANDLER.with(|handler| {
handler
.struct_span_err(
expr_stmt.span,
"NEXT_RSC_ERR_CLIENT_DIRECTIVE_PAREN",
)
.emit()
})
}
}
}
_ => {
// Other expression types.
finished_directives = true;
}
}
None => {
// Not an expression.
finished_directives = true;
}
}
None => {
// Not an expression.
finished_directives = true;
}
}
}
Expand Down
@@ -0,0 +1,7 @@
import "react"

"use client"

export default function () {
return null;
}
@@ -0,0 +1,4 @@
import "react";
export default function () {
return null;
}
@@ -0,0 +1,6 @@

x NEXT_RSC_ERR_CLIENT_DIRECTIVE
,-[input.js:3:1]
3 | "use client"
: ^^^^^^^^^^^^
`----
Expand Up @@ -16,8 +16,6 @@

import "fs"

"use client";

"bar";

// This is a comment.
Expand Down
Expand Up @@ -3,7 +3,6 @@
// This is a comment.
"foo";
import "fs";
"use client";
"bar";
// This is a comment.
1 + 1;
Expand Down
Expand Up @@ -3,9 +3,7 @@ import type { webpack } from 'next/dist/compiled/webpack/webpack'
import { relative } from 'path'
import { SimpleWebpackError } from './simpleWebpackError'

export function formatRSCErrorMessage(
message: string
): null | [string, string] {
function formatRSCErrorMessage(message: string): null | [string, string] {
if (message && /NEXT_RSC_ERR_/.test(message)) {
let formattedMessage = message
let formattedVerboseMessage = ''
Expand All @@ -15,6 +13,9 @@ export function formatRSCErrorMessage(
const NEXT_RSC_ERR_REACT_API = /.+NEXT_RSC_ERR_REACT_API: (.*?)\n/s
const NEXT_RSC_ERR_SERVER_IMPORT = /.+NEXT_RSC_ERR_SERVER_IMPORT: (.*?)\n/s
const NEXT_RSC_ERR_CLIENT_IMPORT = /.+NEXT_RSC_ERR_CLIENT_IMPORT: (.*?)\n/s
const NEXT_RSC_ERR_CLIENT_DIRECTIVE = /.+NEXT_RSC_ERR_CLIENT_DIRECTIVE\n/s
const NEXT_RSC_ERR_CLIENT_DIRECTIVE_PAREN =
/.+NEXT_RSC_ERR_CLIENT_DIRECTIVE_PAREN\n/s

if (NEXT_RSC_ERR_REACT_API.test(message)) {
formattedMessage = message.replace(
Expand Down Expand Up @@ -49,6 +50,18 @@ export function formatRSCErrorMessage(
)
formattedVerboseMessage =
'\n\nOne of these is marked as a client entry with "use client":\n'
} else if (NEXT_RSC_ERR_CLIENT_DIRECTIVE.test(message)) {
formattedMessage = message.replace(
NEXT_RSC_ERR_CLIENT_DIRECTIVE,
`\n\nThe "use client" directive must be placed before other expressions. Move it to the top of the file to resolve this issue.\n\n`
)
formattedVerboseMessage = '\n\nImport path:\n'
} else if (NEXT_RSC_ERR_CLIENT_DIRECTIVE_PAREN.test(message)) {
formattedMessage = message.replace(
NEXT_RSC_ERR_CLIENT_DIRECTIVE_PAREN,
`\n\n"use client" must be a directive, and placed before other expressions. Remove the parentheses and move it to the top of the file to resolve this issue.\n\n`
)
formattedVerboseMessage = '\n\nImport path:\n'
}

return [formattedMessage, formattedVerboseMessage]
Expand All @@ -73,6 +86,7 @@ export function getRscError(
// https://cs.github.com/webpack/webpack/blob/9fcaa243573005d6fdece9a3f8d89a0e8b399613/lib/stats/DefaultStatsFactoryPlugin.js#L414
const visitedModules = new Set()
const moduleTrace = []

let current = module
while (current) {
if (visitedModules.has(current)) break
Expand Down
7 changes: 6 additions & 1 deletion packages/next/taskfile-swc.js
Expand Up @@ -117,7 +117,12 @@ module.exports = function (task) {
// Make sure the output content keeps the `"use client"` directive.
// TODO: Remove this once SWC fixes the issue.
if (/^['"]use client['"]/.test(source)) {
output.code = '"use client";\n' + output.code
output.code =
'"use client";\n' +
output.code
.split('\n')
.map((l) => (/^['"]use client['"]/.test(l) ? '' : l))
.join('\n')
}

// Replace `.ts|.tsx` with `.js` in files with an extension
Expand Down
19 changes: 19 additions & 0 deletions test/e2e/app-dir/rsc-errors.test.ts
Expand Up @@ -107,4 +107,23 @@ describe('app dir - rsc errors', () => {
'The default export is not a React Component in page:'
)
})

it('should throw an error when "use client" is on the top level but after other expressions', async () => {
const pageFile = 'app/swc/use-client/page.js'
const content = await next.readFile(pageFile)
const uncomment = content.replace("// 'use client'", "'use client'")
await next.patchFile(pageFile, uncomment)
const res = await fetchViaHTTP(next.url, '/swc/use-client')
await next.patchFile(pageFile, content)

await check(async () => {
const { status } = await fetchViaHTTP(next.url, '/swc/use-client')
return status
}, /200/)

expect(res.status).toBe(500)
expect(await res.text()).toContain(
'directive must be placed before other expressions'
)
})
})
7 changes: 7 additions & 0 deletions test/e2e/app-dir/rsc-errors/app/swc/use-client/page.js
@@ -0,0 +1,7 @@
import React from 'react'

// 'use client'

export default function Page() {
return 'hello'
}