From ce76d1712e98d7310385c89ec1917242f7c5111c Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Fri, 18 Feb 2022 16:25:10 +0100 Subject: [PATCH] Leverage existing component checking warning for streaming (#34526) ## Bug Fixes: #31993 * Remove the simple component checking in middleware ssr * Leverage existing components checking for Component / App / Document, if any of these component is not valid react type or is undefined nextjs will error in dev mode with redbox. Like above. image - [x] Related issues linked using `fixes #number` - [x] Integration tests added - [x] Errors have helpful link attached, see `contributing.md` --- .../next-middleware-ssr-loader/index.ts | 4 -- packages/next/server/base-server.ts | 2 +- packages/next/server/load-components.ts | 8 ++- packages/next/server/render.tsx | 4 +- test/integration/react-18/test/index.test.js | 69 +++++++++++++------ 5 files changed, 56 insertions(+), 31 deletions(-) diff --git a/packages/next/build/webpack/loaders/next-middleware-ssr-loader/index.ts b/packages/next/build/webpack/loaders/next-middleware-ssr-loader/index.ts index d457fcaa72d3668..870a6b901693a23 100644 --- a/packages/next/build/webpack/loaders/next-middleware-ssr-loader/index.ts +++ b/packages/next/build/webpack/loaders/next-middleware-ssr-loader/index.ts @@ -39,10 +39,6 @@ export default async function middlewareSSRLoader(this: any) { const reactLoadableManifest = self.__REACT_LOADABLE_MANIFEST const rscManifest = self.__RSC_MANIFEST - if (typeof pageMod.default !== 'function') { - throw new Error('Your page must export a \`default\` component') - } - // Set server context self.__server_context = { page: ${JSON.stringify(page)}, diff --git a/packages/next/server/base-server.ts b/packages/next/server/base-server.ts index 77160619770fe7e..19e62dcc11f23bc 100644 --- a/packages/next/server/base-server.ts +++ b/packages/next/server/base-server.ts @@ -1122,7 +1122,7 @@ export default abstract class Server { const isSSG = !!components.getStaticProps const hasServerProps = !!components.getServerSideProps const hasStaticPaths = !!components.getStaticPaths - const hasGetInitialProps = !!(components.Component as any).getInitialProps + const hasGetInitialProps = !!components.Component?.getInitialProps // Toggle whether or not this is a Data request const isDataReq = !!query._nextDataReq && (isSSG || hasServerProps) diff --git a/packages/next/server/load-components.ts b/packages/next/server/load-components.ts index e4c86fee8bcb33e..b4d4b02a0ac0d9d 100644 --- a/packages/next/server/load-components.ts +++ b/packages/next/server/load-components.ts @@ -1,3 +1,8 @@ +import type { + AppType, + DocumentType, + NextComponentType, +} from '../shared/lib/utils' import { BUILD_MANIFEST, REACT_LOADABLE_MANIFEST, @@ -5,7 +10,6 @@ import { import { join } from 'path' import { requirePage } from './require' import { BuildManifest } from './get-page-files' -import { AppType, DocumentType } from '../shared/lib/utils' import { interopDefault } from '../lib/interop-default' import { PageConfig, @@ -22,7 +26,7 @@ export type ManifestItem = { export type ReactLoadableManifest = { [moduleId: string]: ManifestItem } export type LoadComponentsReturnType = { - Component: React.ComponentType + Component: NextComponentType pageConfig: PageConfig buildManifest: BuildManifest reactLoadableManifest: ReactLoadableManifest diff --git a/packages/next/server/render.tsx b/packages/next/server/render.tsx index d18e1f336b006ee..ce5b2a5d9a42794 100644 --- a/packages/next/server/render.tsx +++ b/packages/next/server/render.tsx @@ -545,7 +545,7 @@ export async function renderToHTML( const defaultAppGetInitialProps = App.getInitialProps === (App as any).origGetInitialProps - const hasPageGetInitialProps = !!(Component as any).getInitialProps + const hasPageGetInitialProps = !!(Component as any)?.getInitialProps const pageIsDynamic = isDynamicRoute(pathname) @@ -561,7 +561,7 @@ export async function renderToHTML( 'getServerSideProps', 'getStaticPaths', ]) { - if ((Component as any)[methodName]) { + if ((Component as any)?.[methodName]) { throw new Error( `page ${pathname} ${methodName} ${GSSP_COMPONENT_MEMBER_ERROR}` ) diff --git a/test/integration/react-18/test/index.test.js b/test/integration/react-18/test/index.test.js index 8d68baaef1912b7..2660a9fd20b1e13 100644 --- a/test/integration/react-18/test/index.test.js +++ b/test/integration/react-18/test/index.test.js @@ -11,11 +11,14 @@ import { nextStart, renderViaHTTP, fetchViaHTTP, + hasRedbox, + getRedboxHeader, } from 'next-test-utils' import blocking from './blocking' import concurrent from './concurrent' import basics from './basics' import strictMode from './strict-mode' +import webdriver from 'next-webdriver' // overrides react and react-dom to v18 const nodeArgs = ['-r', join(__dirname, 'require-hook.js')] @@ -23,6 +26,7 @@ const appDir = join(__dirname, '../app') const nextConfig = new File(join(appDir, 'next.config.js')) const dynamicHello = new File(join(appDir, 'components/dynamic-hello.js')) const unwrappedPage = new File(join(appDir, 'pages/suspense/unwrapped.js')) +const invalidPage = new File(join(appDir, 'pages/invalid.js')) const USING_CREATE_ROOT = 'Using the createRoot API for React' @@ -141,21 +145,22 @@ describe('Blocking mode', () => { }) function runTestsAgainstRuntime(runtime) { - describe(`Concurrent mode in the ${runtime} runtime`, () => { - beforeAll(async () => { - nextConfig.replace("// runtime: 'edge'", `runtime: '${runtime}'`) - dynamicHello.replace('suspense = false', `suspense = true`) - // `noSSR` mode will be ignored by suspense - dynamicHello.replace('let ssr', `let ssr = false`) - }) - afterAll(async () => { - nextConfig.restore() - dynamicHello.restore() - }) - - runTests(`runtime is set to '${runtime}'`, (context) => { + runTests( + `Concurrent mode in the ${runtime} runtime`, + (context, env) => { concurrent(context, (p, q) => renderViaHTTP(context.appPort, p, q)) + if (env === 'dev') { + it('should recover after undefined exported as default', async () => { + const browser = await webdriver(context.appPort, '/invalid') + + expect(await hasRedbox(browser)).toBe(true) + expect(await getRedboxHeader(browser)).toMatch( + `Error: The default export is not a React Component in page: "/invalid"` + ) + }) + } + it('should stream to users', async () => { const res = await fetchViaHTTP(context.appPort, '/ssr') expect(res.headers.get('etag')).toBeNull() @@ -189,17 +194,36 @@ function runTestsAgainstRuntime(runtime) { ) expect(res.headers.get('etag')).toBeDefined() }) - }) - }) + }, + { + beforeAll: (env) => { + if (env === 'dev') { + invalidPage.write(`export const value = 1`) + } + nextConfig.replace("// runtime: 'edge'", `runtime: '${runtime}'`) + dynamicHello.replace('suspense = false', `suspense = true`) + // `noSSR` mode will be ignored by suspense + dynamicHello.replace('let ssr', `let ssr = false`) + }, + afterAll: (env) => { + if (env === 'dev') { + invalidPage.delete() + } + nextConfig.restore() + dynamicHello.restore() + }, + } + ) } -function runTest(mode, name, fn) { +function runTest(env, name, fn, options) { const context = { appDir } - describe(`${name} (${mode})`, () => { + describe(`${name} (${env})`, () => { beforeAll(async () => { context.appPort = await findPort() context.stderr = '' - if (mode === 'dev') { + options?.beforeAll(env) + if (env === 'dev') { context.server = await launchApp(context.appDir, context.appPort, { nodeArgs, onStderr(msg) { @@ -217,16 +241,17 @@ function runTest(mode, name, fn) { } }) afterAll(async () => { + options?.afterAll(env) await killApp(context.server) }) - fn(context) + fn(context, env) }) } runTestsAgainstRuntime('edge') runTestsAgainstRuntime('nodejs') -function runTests(name, fn) { - runTest('dev', name, fn) - runTest('prod', name, fn) +function runTests(name, fn, options) { + runTest('dev', name, fn, options) + runTest('prod', name, fn, options) }