From fbc4f3310f34ab52d69acc67dd1231a20c4b14d4 Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Mon, 3 Oct 2022 18:51:40 +0200 Subject: [PATCH 1/3] improve rsc errors --- .../plugins/wellknown-errors-plugin/index.ts | 6 +- .../wellknown-errors-plugin/parseRSC.ts | 90 +++++++++++++++++++ .../webpackModuleError.ts | 13 +++ .../error-overlay/format-webpack-messages.js | 35 -------- .../index/dynamic-imports/dynamic-server.js | 2 + test/e2e/app-dir/index.test.ts | 40 ++++++++- 6 files changed, 149 insertions(+), 37 deletions(-) create mode 100644 packages/next/build/webpack/plugins/wellknown-errors-plugin/parseRSC.ts diff --git a/packages/next/build/webpack/plugins/wellknown-errors-plugin/index.ts b/packages/next/build/webpack/plugins/wellknown-errors-plugin/index.ts index 05557b1fb6f18..953ba57db0e23 100644 --- a/packages/next/build/webpack/plugins/wellknown-errors-plugin/index.ts +++ b/packages/next/build/webpack/plugins/wellknown-errors-plugin/index.ts @@ -11,7 +11,11 @@ export class WellKnownErrorsPlugin { await Promise.all( compilation.errors.map(async (err, i) => { try { - const moduleError = await getModuleBuildError(compilation, err) + const moduleError = await getModuleBuildError( + compiler, + compilation, + err + ) if (moduleError !== false) { compilation.errors[i] = moduleError } diff --git a/packages/next/build/webpack/plugins/wellknown-errors-plugin/parseRSC.ts b/packages/next/build/webpack/plugins/wellknown-errors-plugin/parseRSC.ts new file mode 100644 index 0000000000000..014707775c85d --- /dev/null +++ b/packages/next/build/webpack/plugins/wellknown-errors-plugin/parseRSC.ts @@ -0,0 +1,90 @@ +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] { + if (message && /NEXT_RSC_ERR_/.test(message)) { + let formattedMessage = message + let formattedVerboseMessage = '' + + // Comes from the "React Server Components" transform in SWC, always + // attach the module trace. + 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 + + if (NEXT_RSC_ERR_REACT_API.test(message)) { + formattedMessage = message.replace( + NEXT_RSC_ERR_REACT_API, + `\n\nYou're importing a component that needs $1. It only works in a Client Component but none of its parents are marked with "client", so they're Server Components by default.\n\n` + ) + formattedVerboseMessage = + '\n\nMaybe one of these should be marked as a "client" entry:\n' + } else if (NEXT_RSC_ERR_SERVER_IMPORT.test(message)) { + formattedMessage = message.replace( + NEXT_RSC_ERR_SERVER_IMPORT, + `\n\nYou're importing a component that imports $1. It only works in a Client Component but none of its parents are marked with "client", so they're Server Components by default.\n\n` + ) + formattedVerboseMessage = + '\n\nMaybe one of these should be marked as a "client" entry:\n' + } else if (NEXT_RSC_ERR_CLIENT_IMPORT.test(message)) { + formattedMessage = message.replace( + NEXT_RSC_ERR_CLIENT_IMPORT, + `\n\nYou're importing a component that needs $1. That only works in a Server Component but one of its parents is marked with "client", so it's a Client Component.\n\n` + ) + formattedVerboseMessage = + '\n\nOne of these is marked as a "client" entry:\n' + } + + return [formattedMessage, formattedVerboseMessage] + } + + return null +} + +// Check if the error is specifically related to React Server Components. +// If so, we'll format the error message to be more helpful. +export function getRscError( + fileName: string, + err: Error, + module: any, + compilation: webpack.Compilation, + compiler: webpack.Compiler +): SimpleWebpackError | false { + const formattedError = formatRSCErrorMessage(err.message) + if (!formattedError) return false + + // Get the module trace: + // 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 + visitedModules.add(current) + moduleTrace.push(current) + const origin = compilation.moduleGraph.getIssuer(current) + if (!origin) break + current = origin + } + + const error = new SimpleWebpackError( + fileName, + formattedError[0] + + formattedError[1] + + moduleTrace + .map((m) => + m.resource ? ' ' + relative(compiler.context, m.resource) : '' + ) + .filter(Boolean) + .join('\n') + ) + + // Delete the stack because it's created here. + error.stack = '' + + return error +} diff --git a/packages/next/build/webpack/plugins/wellknown-errors-plugin/webpackModuleError.ts b/packages/next/build/webpack/plugins/wellknown-errors-plugin/webpackModuleError.ts index d53997d80abc0..45697e9eca6d2 100644 --- a/packages/next/build/webpack/plugins/wellknown-errors-plugin/webpackModuleError.ts +++ b/packages/next/build/webpack/plugins/wellknown-errors-plugin/webpackModuleError.ts @@ -8,6 +8,7 @@ import { getScssError } from './parseScss' import { getNotFoundError } from './parseNotFoundError' import { SimpleWebpackError } from './simpleWebpackError' import isError from '../../../../lib/is-error' +import { getRscError } from './parseRSC' function getFileData( compilation: webpack.Compilation, @@ -42,6 +43,7 @@ function getFileData( } export async function getModuleBuildError( + compiler: webpack.Compiler, compilation: webpack.Compilation, input: any ): Promise { @@ -84,5 +86,16 @@ export async function getModuleBuildError( return scss } + const rsc = getRscError( + sourceFilename, + err, + input.module, + compilation, + compiler + ) + if (rsc !== false) { + return rsc + } + return false } diff --git a/packages/next/client/dev/error-overlay/format-webpack-messages.js b/packages/next/client/dev/error-overlay/format-webpack-messages.js index 1e5d73d7fff8a..b091084ae99e1 100644 --- a/packages/next/client/dev/error-overlay/format-webpack-messages.js +++ b/packages/next/client/dev/error-overlay/format-webpack-messages.js @@ -183,41 +183,6 @@ function formatWebpackMessages(json, verbose) { ) } - // TODO: Shall we use invisible characters in the original error - // message as meta information? - if (message && message.message && /NEXT_RSC_ERR_/.test(message.message)) { - // Comes from the "React Server Components" transform in SWC, always - // attach the module trace. - 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 - - if (NEXT_RSC_ERR_REACT_API.test(message.message)) { - message.message = message.message.replace( - NEXT_RSC_ERR_REACT_API, - `\n\nYou're importing a component that needs $1. It only works in a Client Component but none of its parents are marked with "client", so they're Server Components by default.\n\n` - ) - importTraceNote = - '\n\nMaybe one of these should be marked as a "client" entry:\n' - } else if (NEXT_RSC_ERR_SERVER_IMPORT.test(message.message)) { - message.message = message.message.replace( - NEXT_RSC_ERR_SERVER_IMPORT, - `\n\nYou're importing a component that imports $1. It only works in a Client Component but none of its parents are marked with "client", so they're Server Components by default.\n\n` - ) - importTraceNote = - '\n\nMaybe one of these should be marked as a "client" entry:\n' - } else if (NEXT_RSC_ERR_CLIENT_IMPORT.test(message.message)) { - message.message = message.message.replace( - NEXT_RSC_ERR_CLIENT_IMPORT, - `\n\nYou're importing a component that needs $1. That only works in a Server Component but one of its parents is marked with "client", so it's a Client Component.\n\n` - ) - importTraceNote = '\n\nOne of these is marked as a "client" entry:\n' - } - - verbose = true - } return formatMessage(message, verbose, importTraceNote) }) const formattedWarnings = json.warnings.map(function (message) { diff --git a/test/e2e/app-dir/app/app/dashboard/index/dynamic-imports/dynamic-server.js b/test/e2e/app-dir/app/app/dashboard/index/dynamic-imports/dynamic-server.js index 0d9406e90f4a0..36d3a54ad333c 100644 --- a/test/e2e/app-dir/app/app/dashboard/index/dynamic-imports/dynamic-server.js +++ b/test/e2e/app-dir/app/app/dashboard/index/dynamic-imports/dynamic-server.js @@ -1,5 +1,7 @@ import dynamic from 'next/dist/client/components/shared/dynamic' +// import { useEffect } from 'react' + const Dynamic = dynamic(() => import('../text-dynamic-server')) export function NextDynamicServerComponent() { diff --git a/test/e2e/app-dir/index.test.ts b/test/e2e/app-dir/index.test.ts index 522efeead5efb..8177d99982ff7 100644 --- a/test/e2e/app-dir/index.test.ts +++ b/test/e2e/app-dir/index.test.ts @@ -1,7 +1,13 @@ import { createNext, FileRef } from 'e2e-utils' import crypto from 'crypto' import { NextInstance } from 'test/lib/next-modes/base' -import { check, fetchViaHTTP, renderViaHTTP, waitFor } from 'next-test-utils' +import { + check, + fetchViaHTTP, + renderViaHTTP, + waitFor, + File, +} from 'next-test-utils' import path from 'path' import cheerio from 'cheerio' import webdriver from 'next-webdriver' @@ -1624,6 +1630,38 @@ describe('app dir', () => { }) }) }) + + if (isDev) { + describe.only('error overlay', () => { + it('should have error and import path when importing useState in a server component', async () => { + const comp = new File( + path.resolve( + __dirname, + './app/app/dashboard/index/dynamic-imports/dynamic-server.js' + ) + ) + + comp.replace( + `// import { useEffect } from 'react'`, + `import { useEffect } from 'react'` + ) + + const res = await fetchViaHTTP(next.url, '/dashboard/index') + expect(res.status).toBe(500) + const content = await res.text() + + comp.restore() + + expect(content).toInclude( + `You're importing a component that needs useEffect. It only works in a Client Component but none of its parents are marked with \"client\", so they're Server Components by default.` + ) + expect(content).toInclude( + `app/dashboard/index/dynamic-imports/dynamic-server.js` + ) + expect(content).toInclude(`app/dashboard/index/page.js`) + }) + }) + } } runTests() From b379919cbfdcb3a2baa353073d9651f8b7711b0b Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Mon, 3 Oct 2022 19:43:32 +0200 Subject: [PATCH 2/3] remove test --- .../index/dynamic-imports/dynamic-server.js | 2 -- test/e2e/app-dir/index.test.ts | 32 ------------------- 2 files changed, 34 deletions(-) diff --git a/test/e2e/app-dir/app/app/dashboard/index/dynamic-imports/dynamic-server.js b/test/e2e/app-dir/app/app/dashboard/index/dynamic-imports/dynamic-server.js index 36d3a54ad333c..0d9406e90f4a0 100644 --- a/test/e2e/app-dir/app/app/dashboard/index/dynamic-imports/dynamic-server.js +++ b/test/e2e/app-dir/app/app/dashboard/index/dynamic-imports/dynamic-server.js @@ -1,7 +1,5 @@ import dynamic from 'next/dist/client/components/shared/dynamic' -// import { useEffect } from 'react' - const Dynamic = dynamic(() => import('../text-dynamic-server')) export function NextDynamicServerComponent() { diff --git a/test/e2e/app-dir/index.test.ts b/test/e2e/app-dir/index.test.ts index 8177d99982ff7..7c9477efb22bd 100644 --- a/test/e2e/app-dir/index.test.ts +++ b/test/e2e/app-dir/index.test.ts @@ -1630,38 +1630,6 @@ describe('app dir', () => { }) }) }) - - if (isDev) { - describe.only('error overlay', () => { - it('should have error and import path when importing useState in a server component', async () => { - const comp = new File( - path.resolve( - __dirname, - './app/app/dashboard/index/dynamic-imports/dynamic-server.js' - ) - ) - - comp.replace( - `// import { useEffect } from 'react'`, - `import { useEffect } from 'react'` - ) - - const res = await fetchViaHTTP(next.url, '/dashboard/index') - expect(res.status).toBe(500) - const content = await res.text() - - comp.restore() - - expect(content).toInclude( - `You're importing a component that needs useEffect. It only works in a Client Component but none of its parents are marked with \"client\", so they're Server Components by default.` - ) - expect(content).toInclude( - `app/dashboard/index/dynamic-imports/dynamic-server.js` - ) - expect(content).toInclude(`app/dashboard/index/page.js`) - }) - }) - } } runTests() From ddf6b0d20e6e2ade3de4350ae7495bdff07078ee Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Mon, 3 Oct 2022 19:51:58 +0200 Subject: [PATCH 3/3] remove unused import --- test/e2e/app-dir/index.test.ts | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/test/e2e/app-dir/index.test.ts b/test/e2e/app-dir/index.test.ts index 7c9477efb22bd..522efeead5efb 100644 --- a/test/e2e/app-dir/index.test.ts +++ b/test/e2e/app-dir/index.test.ts @@ -1,13 +1,7 @@ import { createNext, FileRef } from 'e2e-utils' import crypto from 'crypto' import { NextInstance } from 'test/lib/next-modes/base' -import { - check, - fetchViaHTTP, - renderViaHTTP, - waitFor, - File, -} from 'next-test-utils' +import { check, fetchViaHTTP, renderViaHTTP, waitFor } from 'next-test-utils' import path from 'path' import cheerio from 'cheerio' import webdriver from 'next-webdriver'