From 931666dd3cdd9e75e971b308379f79074f905250 Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Fri, 11 Feb 2022 19:30:39 +0100 Subject: [PATCH] Fix uncaught error in getInitialProps when `runtime` is set to `nodejs` (#34228) This PR ensures that the test "should render 500 error correctly" doesn't break when `runtime` is set to `nodejs` with `serverComponents` enabled. This test case is now moved to the "basic" suite to ensure it doesn't break in both runtimes. And "should not bundle external imports into client builds for RSC" is enabled for the `nodejs` runtime too. ## Bug - [ ] Related issues linked using `fixes #number` - [x] Integration tests added - [ ] Errors have 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 helpful link attached, see `contributing.md` ## Documentation / Examples - [ ] Make sure the linting passes by running `yarn lint` --- .../loaders/next-flight-server-loader.ts | 23 ++++-- packages/next/server/render.tsx | 7 +- .../test/basic.js | 13 +++- .../test/index.test.js | 77 +++++++------------ .../test/rsc.js | 22 +++++- .../test/utils.js | 13 ++++ 6 files changed, 98 insertions(+), 57 deletions(-) diff --git a/packages/next/build/webpack/loaders/next-flight-server-loader.ts b/packages/next/build/webpack/loaders/next-flight-server-loader.ts index f3243d1dc8c72..f125d562da505 100644 --- a/packages/next/build/webpack/loaders/next-flight-server-loader.ts +++ b/packages/next/build/webpack/loaders/next-flight-server-loader.ts @@ -104,6 +104,12 @@ async function parseImportsInfo( } break } + case 'ExportDefaultExpression': + const exp = node.expression + if (exp.type === 'Identifier') { + defaultExportName = exp.value + } + break default: break } @@ -157,11 +163,18 @@ export default async function transformSource( */ const noop = `export const __rsc_noop__=()=>{${imports.join(';')}}` - const defaultExportNoop = isClientCompilation - ? `export default function ${defaultExportName}(){}\n${defaultExportName}.__next_rsc__=1;` - : defaultExportName - ? `${defaultExportName}.__next_rsc__=1;${defaultExportName}.__webpack_require__=__webpack_require__;` - : '' + + let defaultExportNoop = '' + if (isClientCompilation) { + defaultExportNoop = `export default function ${ + defaultExportName || 'ServerComponent' + }(){}\n${defaultExportName || 'ServerComponent'}.__next_rsc__=1;` + } else { + if (defaultExportName) { + // It's required to have the default export for pages. For other components, it's fine to leave it as is. + defaultExportNoop = `${defaultExportName}.__next_rsc__=1;${defaultExportName}.__webpack_require__=__webpack_require__;` + } + } const transformed = transformedSource + '\n' + noop + '\n' + defaultExportNoop diff --git a/packages/next/server/render.tsx b/packages/next/server/render.tsx index 0d77d18f5f0dc..8daac1499eb00 100644 --- a/packages/next/server/render.tsx +++ b/packages/next/server/render.tsx @@ -467,9 +467,14 @@ export async function renderToHTML( const hasConcurrentFeatures = !!runtime - const isServerComponent = !!serverComponentManifest && hasConcurrentFeatures const OriginalComponent = renderOpts.Component + // We don't need to opt-into the flight inlining logic if the page isn't a RSC. + const isServerComponent = + !!serverComponentManifest && + hasConcurrentFeatures && + (OriginalComponent as any).__next_rsc__ + let Component: React.ComponentType<{}> | ((props: any) => JSX.Element) = renderOpts.Component let serverComponentsInlinedTransformStream: TransformStream | null = diff --git a/test/integration/react-streaming-and-server-components/test/basic.js b/test/integration/react-streaming-and-server-components/test/basic.js index 468108ffe94b1..9cd2bad83bf41 100644 --- a/test/integration/react-streaming-and-server-components/test/basic.js +++ b/test/integration/react-streaming-and-server-components/test/basic.js @@ -1,7 +1,7 @@ import webdriver from 'next-webdriver' import { renderViaHTTP } from 'next-test-utils' -export default async function basic(context) { +export default async function basic(context, { env }) { it('should render 404 error correctly', async () => { const path404HTML = await renderViaHTTP(context.appPort, '/404') const pathNotFoundHTML = await renderViaHTTP(context.appPort, '/not-found') @@ -35,4 +35,15 @@ export default async function basic(context) { expect(hydrationContent).toBe('custom-404-pagenext_streaming_data') }) + + it('should render 500 error correctly', async () => { + const path500HTML = await renderViaHTTP(context.appPort, '/err') + + if (env === 'dev') { + // In dev mode it should show the error popup. + expect(path500HTML).toContain('Error: oops') + } else { + expect(path500HTML).toContain('custom-500-page') + } + }) } diff --git a/test/integration/react-streaming-and-server-components/test/index.test.js b/test/integration/react-streaming-and-server-components/test/index.test.js index 6777fa461c88a..8f8e7e0fb805f 100644 --- a/test/integration/react-streaming-and-server-components/test/index.test.js +++ b/test/integration/react-streaming-and-server-components/test/index.test.js @@ -4,15 +4,21 @@ import { join } from 'path' import fs from 'fs-extra' import webdriver from 'next-webdriver' -import { - File, - fetchViaHTTP, - findPort, - killApp, - renderViaHTTP, -} from 'next-test-utils' +import { fetchViaHTTP, findPort, killApp, renderViaHTTP } from 'next-test-utils' -import { nextBuild, nextStart, nextDev } from './utils' +import { + nextBuild, + nextStart, + nextDev, + appDir, + nativeModuleTestAppDir, + distDir, + documentPage, + appPage, + appServerPage, + error500Page, + nextConfig, +} from './utils' import css from './css' import rsc from './rsc' @@ -20,15 +26,6 @@ import streaming from './streaming' import basic from './basic' import functions from './functions' -const appDir = join(__dirname, '../app') -const nativeModuleTestAppDir = join(__dirname, '../unsupported-native-module') -const distDir = join(__dirname, '../app/.next') -const documentPage = new File(join(appDir, 'pages/_document.jsx')) -const appPage = new File(join(appDir, 'pages/_app.js')) -const appServerPage = new File(join(appDir, 'pages/_app.server.js')) -const error500Page = new File(join(appDir, 'pages/500.js')) -const nextConfig = new File(join(appDir, 'next.config.js')) - const documentWithGip = ` import { Html, Head, Main, NextScript } from 'next/document' @@ -153,14 +150,9 @@ describe('Edge runtime - prod', () => { expect(html).toContain('foo.client') }) - it('should render 500 error correctly', async () => { - const path500HTML = await renderViaHTTP(context.appPort, '/err') - expect(path500HTML).toContain('custom-500-page') - }) - - basic(context) - rsc(context, 'edge') + basic(context, { env: 'prod' }) streaming(context) + rsc(context, { runtime: 'edge', env: 'prod' }) }) describe('Edge runtime - dev', () => { @@ -185,35 +177,16 @@ describe('Edge runtime - dev', () => { expect(content).toMatchInlineSnapshot('"foo.client"') }) - it('should not bundle external imports into client builds for RSC', async () => { - const html = await renderViaHTTP(context.appPort, '/external-imports') - expect(html).toContain('date:') - - const distServerDir = join(distDir, 'static', 'chunks', 'pages') - const bundle = fs - .readFileSync(join(distServerDir, 'external-imports.js')) - .toString() - - expect(bundle).not.toContain('moment') - }) - - it('should render 500 error correctly', async () => { - const path500HTML = await renderViaHTTP(context.appPort, '/err') - - // In dev mode it should show the error popup. - expect(path500HTML).toContain('Error: oops') - }) - - basic(context) - rsc(context, 'edge') + basic(context, { env: 'dev' }) streaming(context) + rsc(context, { runtime: 'edge', env: 'dev' }) }) const nodejsRuntimeBasicSuite = { runTests: (context, env) => { - basic(context) + basic(context, { env }) streaming(context) - rsc(context, 'nodejs') + rsc(context, { runtime: 'nodejs' }) if (env === 'prod') { it('should generate middleware SSR manifests for Node.js', async () => { @@ -241,8 +214,14 @@ const nodejsRuntimeBasicSuite = { }) } }, - beforeAll: () => nextConfig.replace("runtime: 'edge'", "runtime: 'nodejs'"), - afterAll: () => nextConfig.restore(), + beforeAll: () => { + error500Page.write(page500) + nextConfig.replace("runtime: 'edge'", "runtime: 'nodejs'") + }, + afterAll: () => { + error500Page.delete() + nextConfig.restore() + }, } const customAppPageSuite = { diff --git a/test/integration/react-streaming-and-server-components/test/rsc.js b/test/integration/react-streaming-and-server-components/test/rsc.js index 77cee8d228c47..988c25f5e7679 100644 --- a/test/integration/react-streaming-and-server-components/test/rsc.js +++ b/test/integration/react-streaming-and-server-components/test/rsc.js @@ -2,13 +2,17 @@ import webdriver from 'next-webdriver' import cheerio from 'cheerio' import { renderViaHTTP, check } from 'next-test-utils' +import { join } from 'path' +import fs from 'fs-extra' + +import { distDir } from './utils' function getNodeBySelector(html, selector) { const $ = cheerio.load(html) return $(selector) } -export default function (context, runtime) { +export default function (context, { runtime, env }) { it('should render server components correctly', async () => { const homeHTML = await renderViaHTTP(context.appPort, '/', null, { headers: { @@ -72,6 +76,22 @@ export default function (context, runtime) { }) } + // For prod build, the directory contains the build ID so it's not deterministic. + // Only enable it for dev for now. + if (env === 'dev') { + it('should not bundle external imports into client builds for RSC', async () => { + const html = await renderViaHTTP(context.appPort, '/external-imports') + expect(html).toContain('date:') + + const distServerDir = join(distDir, 'static', 'chunks', 'pages') + const bundle = fs + .readFileSync(join(distServerDir, 'external-imports.js')) + .toString() + + expect(bundle).not.toContain('moment') + }) + } + it('should handle multiple named exports correctly', async () => { const clientExportsHTML = await renderViaHTTP( context.appPort, diff --git a/test/integration/react-streaming-and-server-components/test/utils.js b/test/integration/react-streaming-and-server-components/test/utils.js index 99c6056bb0297..4e51c063272cd 100644 --- a/test/integration/react-streaming-and-server-components/test/utils.js +++ b/test/integration/react-streaming-and-server-components/test/utils.js @@ -1,5 +1,6 @@ import { join } from 'path' import { + File, launchApp, nextBuild as _nextBuild, nextStart as _nextStart, @@ -7,6 +8,18 @@ import { const nodeArgs = ['-r', join(__dirname, '../../react-18/test/require-hook.js')] +export const appDir = join(__dirname, '../app') +export const nativeModuleTestAppDir = join( + __dirname, + '../unsupported-native-module' +) +export const distDir = join(__dirname, '../app/.next') +export const documentPage = new File(join(appDir, 'pages/_document.jsx')) +export const appPage = new File(join(appDir, 'pages/_app.js')) +export const appServerPage = new File(join(appDir, 'pages/_app.server.js')) +export const error500Page = new File(join(appDir, 'pages/500.js')) +export const nextConfig = new File(join(appDir, 'next.config.js')) + export async function nextBuild(dir, options) { return await _nextBuild(dir, [], { ...options,