From e029ace5edd5a46544c2817e95964a8358133ea9 Mon Sep 17 00:00:00 2001 From: Jiachi Liu Date: Sat, 6 Nov 2021 12:27:40 +0100 Subject: [PATCH] Fix custom 404 page when concurrentFeatures is enabled (#31059) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit x-ref: https://github.com/vercel/next.js/issues/30424#issuecomment-955615781 This fix the custom 404 is not rendering properly and can’t be built in web runtime when `concurrentFeatures` is enabled. We force 404 page to be rendered outside of middleware ssr. Then it could be the real fallback 404 page in next-server when any routes is not macthed. Will check 500 related after #31057 is landed. ## Bug - [x] Related to #30989 - [x] Integration tests added --- packages/next/build/entries.ts | 18 +++----- packages/next/build/index.ts | 20 +++++---- packages/next/build/utils.ts | 9 ++++ .../next-middleware-ssr-loader/index.ts | 18 ++++---- .../webpack/plugins/middleware-plugin.ts | 5 +-- packages/next/server/dev/hot-reloader.ts | 42 ++++++++++++------- packages/next/server/dev/next-dev-server.ts | 7 +--- .../server/dev/on-demand-entry-handler.ts | 14 +++++-- .../app/pages/404.js | 3 ++ .../test/index.test.js | 10 +++++ 10 files changed, 90 insertions(+), 56 deletions(-) create mode 100644 test/integration/react-streaming-and-server-components/app/pages/404.js diff --git a/packages/next/build/entries.ts b/packages/next/build/entries.ts index c5d58c46cd10e0d..be2f0b3e8236e48 100644 --- a/packages/next/build/entries.ts +++ b/packages/next/build/entries.ts @@ -12,7 +12,7 @@ import { ClientPagesLoaderOptions } from './webpack/loaders/next-client-pages-lo import { ServerlessLoaderQuery } from './webpack/loaders/next-serverless-loader' import { LoadedEnvFiles } from '@next/env' import { NextConfigComplete } from '../server/config-shared' -import { isFlightPage } from './utils' +import { isCustomErrorPage, isFlightPage, isReservedPage } from './utils' import { ssrEntries } from './webpack/plugins/middleware-plugin' import type { webpack5 } from 'next/dist/compiled/webpack/webpack' import { MIDDLEWARE_SSR_RUNTIME_WEBPACK } from '../shared/lib/constants' @@ -136,7 +136,10 @@ export function createEntrypoints( const serverBundlePath = posix.join('pages', bundleFile) const isLikeServerless = isTargetLikeServerless(target) + const isReserved = isReservedPage(page) + const isCustomError = isCustomErrorPage(page) const isFlight = isFlightPage(config, absolutePagePath) + const webServerRuntime = !!config.experimental.concurrentFeatures if (page.match(MIDDLEWARE_ROUTE)) { @@ -151,11 +154,7 @@ export function createEntrypoints( return } - if ( - webServerRuntime && - !(page === '/_app' || page === '/_error' || page === '/_document') && - !isApiRoute - ) { + if (webServerRuntime && !isReserved && !isCustomError && !isApiRoute) { ssrEntries.set(clientBundlePath, { requireFlightManifest: isFlight }) serverWeb[serverBundlePath] = finalizeEntrypoint({ name: '[name].js', @@ -184,12 +183,7 @@ export function createEntrypoints( serverlessLoaderOptions )}!` } else if (isApiRoute || target === 'server') { - if ( - !webServerRuntime || - page === '/_document' || - page === '/_app' || - page === '/_error' - ) { + if (!webServerRuntime || isReserved || isCustomError) { server[serverBundlePath] = [absolutePagePath] } } else if ( diff --git a/packages/next/build/index.ts b/packages/next/build/index.ts index 19961b9826be1e7..aaff62ec7fca273 100644 --- a/packages/next/build/index.ts +++ b/packages/next/build/index.ts @@ -91,6 +91,8 @@ import { printTreeView, getCssFilePaths, getUnresolvedModuleFromError, + isReservedPage, + isCustomErrorPage, } from './utils' import getBaseWebpackConfig from './webpack-config' import { PagesManifest } from './webpack/plugins/pages-manifest-plugin' @@ -102,8 +104,6 @@ import { TelemetryPlugin } from './webpack/plugins/telemetry-plugin' import { MiddlewareManifest } from './webpack/plugins/middleware-plugin' import type { webpack5 as webpack } from 'next/dist/compiled/webpack/webpack' -const RESERVED_PAGE = /^\/(_app|_error|_document|api(\/|$))/ - export type SsgRoute = { initialRevalidateSeconds: number | false srcRoute: string | null @@ -465,7 +465,7 @@ export default async function build( (page) => !isDynamicRoute(page) && !page.match(MIDDLEWARE_ROUTE) && - !page.match(RESERVED_PAGE) + !isReservedPage(page) ) .map(pageToRoute), dataRoutes: [], @@ -916,7 +916,7 @@ export default async function build( if ( !isMiddlewareRoute && - !page.match(RESERVED_PAGE) && + !isReservedPage(page) && !hasConcurrentFeatures ) { try { @@ -1029,7 +1029,8 @@ export default async function build( isWebSsr: hasConcurrentFeatures && !isMiddlewareRoute && - !page.match(RESERVED_PAGE), + !isReservedPage(page) && + !isCustomErrorPage(page), isHybridAmp, ssgPageRoutes, initialRevalidateSeconds: false, @@ -1318,7 +1319,9 @@ export default async function build( // Since custom _app.js can wrap the 404 page we have to opt-out of static optimization if it has getInitialProps // Only export the static 404 when there is no /_error present const useStatic404 = - !customAppGetInitialProps && (!hasNonStaticErrorPage || hasPages404) + !hasConcurrentFeatures && + !customAppGetInitialProps && + (!hasNonStaticErrorPage || hasPages404) if (invalidPages.size > 0) { const err = new Error( @@ -1383,7 +1386,10 @@ export default async function build( const combinedPages = [...staticPages, ...ssgPages] - if (combinedPages.length > 0 || useStatic404 || useDefaultStatic500) { + if ( + !hasConcurrentFeatures && + (combinedPages.length > 0 || useStatic404 || useDefaultStatic500) + ) { const staticGenerationSpan = nextBuildSpan.traceChild('static-generation') await staticGenerationSpan.traceAsyncFn(async () => { detectConflictingPaths( diff --git a/packages/next/build/utils.ts b/packages/next/build/utils.ts index 3b87294fd8b09d4..54a48944011227d 100644 --- a/packages/next/build/utils.ts +++ b/packages/next/build/utils.ts @@ -37,6 +37,7 @@ import { NextConfigComplete } from '../server/config-shared' import isError from '../lib/is-error' const { builtinModules } = require('module') +const RESERVED_PAGE = /^\/(_app|_error|_document|api(\/|$))/ const fileGzipStats: { [k: string]: Promise | undefined } = {} const fsStatGzip = (file: string) => { const cached = fileGzipStats[file] @@ -1144,3 +1145,11 @@ export function getUnresolvedModuleFromError( const [, moduleName] = error.match(moduleErrorRegex) || [] return builtinModules.find((item: string) => item === moduleName) } + +export function isReservedPage(page: string) { + return RESERVED_PAGE.test(page) +} + +export function isCustomErrorPage(page: string) { + return page === '/404' || page === '/500' +} 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 07f6dfd0861826f..b0ee97c93888f67 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 @@ -96,13 +96,13 @@ export default async function middlewareRSCLoader(this: any) { createElement(FlightWrapper, props) ) }` - : ` - const Component = Page` + : `const Component = Page` } async function render(request) { const url = request.nextUrl - const query = Object.fromEntries(url.searchParams) + const { pathname, searchParams } = url + const query = Object.fromEntries(searchParams) // Preflight request if (request.method === 'HEAD') { @@ -122,9 +122,9 @@ export default async function middlewareRSCLoader(this: any) { wrapReadable( renderFlight({ router: { - route: url.pathname, - asPath: url.pathname, - pathname: url.pathname, + route: pathname, + asPath: pathname, + pathname: pathname, query, } }) @@ -165,9 +165,9 @@ export default async function middlewareRSCLoader(this: any) { try { const result = await renderToHTML( - { url: url.pathname }, + { url: pathname }, {}, - url.pathname, + pathname, query, renderOpts ) @@ -177,7 +177,7 @@ export default async function middlewareRSCLoader(this: any) { }) } catch (err) { return new Response( - (err || 'An error occurred while rendering ' + url.pathname + '.').toString(), + (err || 'An error occurred while rendering ' + pathname + '.').toString(), { status: 500, headers: { 'x-middleware-ssr': '1' } diff --git a/packages/next/build/webpack/plugins/middleware-plugin.ts b/packages/next/build/webpack/plugins/middleware-plugin.ts index 579cfb8a230da3f..a42354f27a04b97 100644 --- a/packages/next/build/webpack/plugins/middleware-plugin.ts +++ b/packages/next/build/webpack/plugins/middleware-plugin.ts @@ -100,9 +100,8 @@ export default class MiddlewarePlugin { ) middlewareManifest.clientInfo = middlewareManifest.sortedMiddleware.map( (key) => { - const ssrEntryInfo = ssrEntries.get( - middlewareManifest.middleware[key].name - ) + const middleware = middlewareManifest.middleware[key] + const ssrEntryInfo = ssrEntries.get(middleware.name) return [key, !!ssrEntryInfo] } ) diff --git a/packages/next/server/dev/hot-reloader.ts b/packages/next/server/dev/hot-reloader.ts index db7c7a700ad47be..7a0164864c2c5b9 100644 --- a/packages/next/server/dev/hot-reloader.ts +++ b/packages/next/server/dev/hot-reloader.ts @@ -29,7 +29,12 @@ import { fileExists } from '../../lib/file-exists' import { ClientPagesLoaderOptions } from '../../build/webpack/loaders/next-client-pages-loader' import { ssrEntries } from '../../build/webpack/plugins/middleware-plugin' import { stringify } from 'querystring' -import { difference, isFlightPage } from '../../build/utils' +import { + difference, + isCustomErrorPage, + isFlightPage, + isReservedPage, +} from '../../build/utils' import { NextConfigComplete } from '../config-shared' import { CustomRoutes } from '../../lib/load-custom-routes' import { DecodeError } from '../../shared/lib/utils' @@ -441,11 +446,18 @@ export default class HotReloader { await Promise.all( Object.keys(entries).map(async (pageKey) => { const isClientKey = pageKey.startsWith('client') + const isServerWebKey = pageKey.startsWith('server-web') if (isClientKey !== isClientCompilation) return + if (isServerWebKey !== isServerWebCompilation) return const page = pageKey.slice( - isClientKey ? 'client'.length : 'server'.length + isClientKey + ? 'client'.length + : isServerWebKey + ? 'server-web'.length + : 'server'.length ) - const isMiddleware = page.match(MIDDLEWARE_ROUTE) + const isMiddleware = !!page.match(MIDDLEWARE_ROUTE) + if (isClientCompilation && page.match(API_ROUTE) && !isMiddleware) { return } @@ -464,11 +476,18 @@ export default class HotReloader { return } + const isCustomError = isCustomErrorPage(page) + const isReserved = isReservedPage(page) const isServerComponent = this.hasServerComponents && isFlightPage(this.config, absolutePagePath) - if (isServerCompilation && this.webServerRuntime && !isApiRoute) { + if ( + isServerCompilation && + this.webServerRuntime && + !isApiRoute && + !isCustomError + ) { return } @@ -496,22 +515,13 @@ export default class HotReloader { ssrEntries.set(bundlePath, { requireFlightManifest: true }) } else if ( this.webServerRuntime && - !( - page === '/_app' || - page === '/_error' || - page === '/_document' - ) + !isReserved && + !isCustomError ) { ssrEntries.set(bundlePath, { requireFlightManifest: false }) } } else if (isServerWebCompilation) { - if ( - !( - page === '/_app' || - page === '/_error' || - page === '/_document' - ) - ) { + if (!isReserved) { entrypoints[bundlePath] = finalizeEntrypoint({ name: '[name].js', value: `next-middleware-ssr-loader?${stringify({ diff --git a/packages/next/server/dev/next-dev-server.ts b/packages/next/server/dev/next-dev-server.ts index 03ac4dec26d562e..339d5e183bfcece 100644 --- a/packages/next/server/dev/next-dev-server.ts +++ b/packages/next/server/dev/next-dev-server.ts @@ -59,6 +59,7 @@ import isError from '../../lib/is-error' import { getMiddlewareRegex } from '../../shared/lib/router/utils/get-middleware-regex' import type { FetchEventResult } from '../web/types' import type { ParsedNextUrl } from '../../shared/lib/router/utils/parse-next-url' +import { isCustomErrorPage, isReservedPage } from '../../build/utils' // Load ReactDevOverlay only when needed let ReactDevOverlayImpl: React.FunctionComponent @@ -272,11 +273,7 @@ export default class DevServer extends Server { ssrMiddleware.add(pageName) } else if ( isWebServerRuntime && - !( - pageName === '/_app' || - pageName === '/_error' || - pageName === '/_document' - ) + !(isReservedPage(pageName) || isCustomErrorPage(pageName)) ) { routedMiddleware.push(pageName) ssrMiddleware.add(pageName) diff --git a/packages/next/server/dev/on-demand-entry-handler.ts b/packages/next/server/dev/on-demand-entry-handler.ts index 493222349d0f180..0db64b4fef23ad9 100644 --- a/packages/next/server/dev/on-demand-entry-handler.ts +++ b/packages/next/server/dev/on-demand-entry-handler.ts @@ -9,12 +9,13 @@ import { API_ROUTE, MIDDLEWARE_ROUTE } from '../../lib/constants' import { reportTrigger } from '../../build/output' import type ws from 'ws' import { NextConfigComplete } from '../config-shared' +import { isCustomErrorPage } from '../../build/utils' export const ADDED = Symbol('added') export const BUILDING = Symbol('building') export const BUILT = Symbol('built') -export let entries: { +export const entries: { [page: string]: { bundlePath: string absolutePagePath: string @@ -204,6 +205,7 @@ export default function onDemandEntryHandler( const isMiddleware = normalizedPage.match(MIDDLEWARE_ROUTE) const isApiRoute = normalizedPage.match(API_ROUTE) && !isMiddleware const isServerWeb = !!nextConfig.experimental.concurrentFeatures + const isCustomError = isCustomErrorPage(page) let entriesChanged = false const addPageEntry = (type: 'client' | 'server' | 'server-web') => { @@ -242,20 +244,24 @@ export default function onDemandEntryHandler( }) } + const isClientOrMiddleware = clientOnly || isMiddleware + const promise = isApiRoute ? addPageEntry('server') - : clientOnly || isMiddleware + : isClientOrMiddleware ? addPageEntry('client') : Promise.all([ addPageEntry('client'), - addPageEntry(isServerWeb ? 'server-web' : 'server'), + addPageEntry( + isServerWeb && !isCustomError ? 'server-web' : 'server' + ), ]) if (entriesChanged) { reportTrigger( isApiRoute ? `${normalizedPage} (server only)` - : clientOnly || isMiddleware + : isClientOrMiddleware ? `${normalizedPage} (client only)` : normalizedPage ) diff --git a/test/integration/react-streaming-and-server-components/app/pages/404.js b/test/integration/react-streaming-and-server-components/app/pages/404.js new file mode 100644 index 000000000000000..92471c97e449d05 --- /dev/null +++ b/test/integration/react-streaming-and-server-components/app/pages/404.js @@ -0,0 +1,3 @@ +export default function Page404() { + return 'custom-404-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 c8f5e4999bb21e1..d8dcfe043d2ea2e 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 @@ -140,6 +140,7 @@ describe('concurrentFeatures - prod', () => { ]) { expect(content.clientInfo).toContainEqual(item) } + expect(content.clientInfo).not.toContainEqual([['/404', true]]) }) it('should support React.lazy and dynamic imports', async () => { @@ -215,11 +216,20 @@ async function runBasicTests(context) { '/routes/dynamic2' ) + const path404HTML = await renderViaHTTP(context.appPort, '/404') + const pathNotFoundHTML = await renderViaHTTP( + context.appPort, + '/this-is-not-found' + ) + expect(homeHTML).toContain('thisistheindexpage.server') expect(homeHTML).toContain('foo.client') expect(dynamicRouteHTML1).toContain('[pid]') expect(dynamicRouteHTML2).toContain('[pid]') + + expect(path404HTML).toContain('custom-404-page') + expect(pathNotFoundHTML).toContain('custom-404-page') }) it('should suspense next/link on server side', async () => {