From 21c9d834a971339af71b42be8a7bc64076dda3f7 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Thu, 22 Oct 2020 02:11:24 -0500 Subject: [PATCH 1/2] Update fallback 404 handling to prevent reload loop --- packages/next/build/entries.ts | 1 + .../webpack/loaders/next-serverless-loader.ts | 52 +++++++++++++++---- .../next/next-server/lib/router/router.ts | 44 +++++++++++----- .../next/next-server/server/next-server.ts | 10 ---- packages/next/next-server/server/render.tsx | 1 - .../i18n-support/test/index.test.js | 2 +- 6 files changed, 75 insertions(+), 35 deletions(-) diff --git a/packages/next/build/entries.ts b/packages/next/build/entries.ts index 2964dd6b4cf1e..5702c24e9c5b7 100644 --- a/packages/next/build/entries.ts +++ b/packages/next/build/entries.ts @@ -79,6 +79,7 @@ export function createEntrypoints( absoluteAppPath: pages['/_app'], absoluteDocumentPath: pages['/_document'], absoluteErrorPath: pages['/_error'], + absolute404Path: pages['/404'] || '', distDir: DOT_NEXT_ALIAS, buildId, assetPrefix: config.assetPrefix, diff --git a/packages/next/build/webpack/loaders/next-serverless-loader.ts b/packages/next/build/webpack/loaders/next-serverless-loader.ts index 739a40d3ce747..669594fdb2b3b 100644 --- a/packages/next/build/webpack/loaders/next-serverless-loader.ts +++ b/packages/next/build/webpack/loaders/next-serverless-loader.ts @@ -19,6 +19,7 @@ export type ServerlessLoaderQuery = { absoluteAppPath: string absoluteDocumentPath: string absoluteErrorPath: string + absolute404Path: string buildId: string assetPrefix: string generateEtags: string @@ -44,6 +45,7 @@ const nextServerlessLoader: loader.Loader = function () { absoluteAppPath, absoluteDocumentPath, absoluteErrorPath, + absolute404Path, generateEtags, poweredByHeader, basePath, @@ -494,6 +496,7 @@ const nextServerlessLoader: loader.Loader = function () { export async function renderReqToHTML(req, res, renderMode, _renderOpts, _params) { let Document let Error + let NotFound ;[ getStaticProps, getServerSideProps, @@ -502,7 +505,8 @@ const nextServerlessLoader: loader.Loader = function () { App, config, { default: Document }, - { default: Error } + { default: Error }, + ${absolute404Path ? `{ default: NotFound }, ` : ''} ] = await Promise.all([ getStaticProps, getServerSideProps, @@ -511,7 +515,8 @@ const nextServerlessLoader: loader.Loader = function () { App, config, require('${absoluteDocumentPath}'), - require('${absoluteErrorPath}') + require('${absoluteErrorPath}'), + ${absolute404Path ? `require("${absolute404Path}"),` : ''} ]) const fromExport = renderMode === 'export' || renderMode === true; @@ -767,14 +772,41 @@ const nextServerlessLoader: loader.Loader = function () { if (!renderMode) { if (_nextData || getStaticProps || getServerSideProps) { - sendPayload(req, res, _nextData ? JSON.stringify(renderOpts.pageData) : result, _nextData ? 'json' : 'html', ${ - generateEtags === 'true' ? true : false - }, { - private: isPreviewMode, - stateful: !!getServerSideProps, - revalidate: renderOpts.revalidate, - }) - return null + if (renderOpts.ssgNotFound) { + res.statusCode = 404 + + const NotFoundComponent = ${ + absolute404Path ? 'NotFound' : 'Error' + } + + const errPathname = "${absolute404Path ? '/404' : '/_error'}" + + const result = await renderToHTML(req, res, errPathname, parsedUrl.query, Object.assign({}, options, { + getStaticProps: undefined, + getStaticPaths: undefined, + getServerSideProps: undefined, + Component: NotFoundComponent, + err: undefined + })) + + sendPayload(req, res, result, 'html', ${ + generateEtags === 'true' ? true : false + }, { + private: isPreviewMode, + stateful: !!getServerSideProps, + revalidate: renderOpts.revalidate, + }) + return null + } else { + sendPayload(req, res, _nextData ? JSON.stringify(renderOpts.pageData) : result, _nextData ? 'json' : 'html', ${ + generateEtags === 'true' ? true : false + }, { + private: isPreviewMode, + stateful: !!getServerSideProps, + revalidate: renderOpts.revalidate, + }) + return null + } } } else if (isPreviewMode) { res.setHeader( diff --git a/packages/next/next-server/lib/router/router.ts b/packages/next/next-server/lib/router/router.ts index e027649823d03..d38a746b25cbc 100644 --- a/packages/next/next-server/lib/router/router.ts +++ b/packages/next/next-server/lib/router/router.ts @@ -337,7 +337,7 @@ function fetchNextData(dataHref: string, isServerRender: boolean) { // on a client-side transition. Otherwise, we'd get into an infinite // loop. - if (!isServerRender || err.message === 'SSG Data NOT_FOUND') { + if (!isServerRender) { markLoadingError(err) } throw err @@ -907,13 +907,6 @@ export default class Router implements BaseRouter { // 3. Internal error while loading the page // So, doing a hard reload is the proper way to deal with this. - if (process.env.NODE_ENV === 'development') { - // append __next404 query to prevent fallback from being re-served - // on reload in development - if (err.message === SSG_DATA_NOT_FOUND_ERROR && this.isSsr) { - as += `${as.indexOf('?') > -1 ? '&' : '?'}__next404=1` - } - } window.location.href = as // Changing the URL doesn't block executing the current code path. @@ -922,14 +915,34 @@ export default class Router implements BaseRouter { } try { - const { page: Component, styleSheets } = await this.fetchComponent( - '/_error' - ) + let Component: ComponentType + let styleSheets: StyleSheetTuple[] + const ssg404 = err.message === SSG_DATA_NOT_FOUND_ERROR + + if (ssg404) { + try { + ;({ page: Component, styleSheets } = await this.fetchComponent( + '/404' + )) + } catch (_err) { + // non-fatal fallback to _error + } + } + + if ( + typeof Component! === 'undefined' || + typeof styleSheets! === 'undefined' + ) { + ;({ page: Component, styleSheets } = await this.fetchComponent( + '/_error' + )) + } + const routeInfo: PrivateRouteInfo = { Component, styleSheets, - err, - error: err, + err: ssg404 ? undefined : err, + error: ssg404 ? undefined : err, } try { @@ -938,6 +951,11 @@ export default class Router implements BaseRouter { pathname, query, } as any) + + if (ssg404 && routeInfo.props && routeInfo.props.pageProps) { + routeInfo.props.pageProps.statusCode = 404 + } + console.log(routeInfo) } catch (gipErr) { console.error('Error in error page `getInitialProps`: ', gipErr) routeInfo.props = {} diff --git a/packages/next/next-server/server/next-server.ts b/packages/next/next-server/server/next-server.ts index 4102a0f48c2e2..9070e5aef4192 100644 --- a/packages/next/next-server/server/next-server.ts +++ b/packages/next/next-server/server/next-server.ts @@ -1144,7 +1144,6 @@ export default class Server { ...(components.getStaticProps ? { amp: query.amp, - __next404: query.__next404, _nextDataReq: query._nextDataReq, __nextLocale: query.__nextLocale, } @@ -1270,15 +1269,6 @@ export default class Server { query.amp ? '.amp' : '' }` - // In development we use a __next404 query to allow signaling we should - // render the 404 page after attempting to fetch the _next/data for a - // fallback page since the fallback page will always be available after - // reload and we don't want to re-serve it and instead want to 404. - if (this.renderOpts.dev && isSSG && query.__next404) { - delete query.__next404 - throw new NoFallbackError() - } - // Complete the response with cached data if its present const cachedData = ssgCacheKey ? await this.incrementalCache.get(ssgCacheKey) diff --git a/packages/next/next-server/server/render.tsx b/packages/next/next-server/server/render.tsx index 6a5de13378f82..ff23d179b3135 100644 --- a/packages/next/next-server/server/render.tsx +++ b/packages/next/next-server/server/render.tsx @@ -414,7 +414,6 @@ export async function renderToHTML( const isFallback = !!query.__nextFallback delete query.__nextFallback delete query.__nextLocale - delete query.__next404 const isSSG = !!getStaticProps const isBuildTimeSSG = isSSG && renderOpts.nextExport diff --git a/test/integration/i18n-support/test/index.test.js b/test/integration/i18n-support/test/index.test.js index 74ce600571973..eb5cffe210108 100644 --- a/test/integration/i18n-support/test/index.test.js +++ b/test/integration/i18n-support/test/index.test.js @@ -697,7 +697,7 @@ function runTests(isDev) { true ) expect(parsedUrl.pathname).toBe('/en/not-found/fallback/first') - expect(parsedUrl.query).toEqual(isDev ? { __next404: '1' } : {}) + expect(parsedUrl.query).toEqual({}) if (isDev) { // make sure page doesn't reload un-necessarily in development From 4c8f37529cd3c2f625ca25552fa3a49807a3df00 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Thu, 22 Oct 2020 02:35:04 -0500 Subject: [PATCH 2/2] Update build-output test --- test/integration/build-output/test/index.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/integration/build-output/test/index.test.js b/test/integration/build-output/test/index.test.js index a1c77da767cb0..7e32ab2dbb4f8 100644 --- a/test/integration/build-output/test/index.test.js +++ b/test/integration/build-output/test/index.test.js @@ -95,16 +95,16 @@ describe('Build Output', () => { expect(indexSize.endsWith('B')).toBe(true) // should be no bigger than 60.8 kb - expect(parseFloat(indexFirstLoad) - 61).toBeLessThanOrEqual(0) + expect(parseFloat(indexFirstLoad) - 61.2).toBeLessThanOrEqual(0) expect(indexFirstLoad.endsWith('kB')).toBe(true) expect(parseFloat(err404Size) - 3.5).toBeLessThanOrEqual(0) expect(err404Size.endsWith('kB')).toBe(true) - expect(parseFloat(err404FirstLoad) - 64.2).toBeLessThanOrEqual(0) + expect(parseFloat(err404FirstLoad) - 64.4).toBeLessThanOrEqual(0) expect(err404FirstLoad.endsWith('kB')).toBe(true) - expect(parseFloat(sharedByAll) - 60.8).toBeLessThanOrEqual(0) + expect(parseFloat(sharedByAll) - 61).toBeLessThanOrEqual(0) expect(sharedByAll.endsWith('kB')).toBe(true) if (_appSize.endsWith('kB')) {