From 5be2917c471cfebdedd5da4f3496378d151901d5 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Wed, 19 Feb 2020 14:55:26 -0600 Subject: [PATCH] [now-next] Update 404 handling for pages/404 (#3697) This adds handling for treating `pages/404.js` as the custom error page in Next.js to allow for more flexible auto static optimization on the 404 page. This makes sure we handle `pages/404.js` being a lambda due to `_app` having `getInitialProps` and also makes sure that visiting `/404` doesn't respond with a 200 status code ~We can add tests for this behavior after the below PR has been released in a canary of Next.js~ x-ref: https://github.com/zeit/next.js/pull/10329 x-ref: https://github.com/zeit/next.js/pull/10593 --- packages/now-next/src/index.ts | 46 +++++++++++++++---- packages/now-next/src/utils.ts | 1 + .../test/fixtures/05-spr-support/package.json | 2 +- .../test/fixtures/19-pages-404/next.config.js | 5 ++ .../test/fixtures/19-pages-404/now.json | 24 ++++++++++ .../test/fixtures/19-pages-404/package.json | 7 +++ .../test/fixtures/19-pages-404/pages/404.js | 1 + .../test/fixtures/19-pages-404/pages/index.js | 1 + .../20-pages-404-lambda/next.config.js | 5 ++ .../fixtures/20-pages-404-lambda/now.json | 18 ++++++++ .../fixtures/20-pages-404-lambda/package.json | 7 +++ .../fixtures/20-pages-404-lambda/pages/404.js | 1 + .../20-pages-404-lambda/pages/_app.js | 5 ++ .../20-pages-404-lambda/pages/index.js | 1 + 14 files changed, 114 insertions(+), 10 deletions(-) create mode 100644 packages/now-next/test/fixtures/19-pages-404/next.config.js create mode 100644 packages/now-next/test/fixtures/19-pages-404/now.json create mode 100644 packages/now-next/test/fixtures/19-pages-404/package.json create mode 100644 packages/now-next/test/fixtures/19-pages-404/pages/404.js create mode 100644 packages/now-next/test/fixtures/19-pages-404/pages/index.js create mode 100644 packages/now-next/test/fixtures/20-pages-404-lambda/next.config.js create mode 100644 packages/now-next/test/fixtures/20-pages-404-lambda/now.json create mode 100644 packages/now-next/test/fixtures/20-pages-404-lambda/package.json create mode 100644 packages/now-next/test/fixtures/20-pages-404-lambda/pages/404.js create mode 100644 packages/now-next/test/fixtures/20-pages-404-lambda/pages/_app.js create mode 100644 packages/now-next/test/fixtures/20-pages-404-lambda/pages/index.js diff --git a/packages/now-next/src/index.ts b/packages/now-next/src/index.ts index 1cae6b60849..fd8bceb30b0 100644 --- a/packages/now-next/src/index.ts +++ b/packages/now-next/src/index.ts @@ -340,6 +340,8 @@ export const build = async ({ const redirects: Route[] = []; const nextBasePathRoute: Route[] = []; let nextBasePath: string | undefined; + // whether they have enabled pages/404.js as the custom 404 page + let hasPages404 = false; if (routesManifest) { switch (routesManifest.version) { @@ -352,6 +354,10 @@ export const build = async ({ headers.push(...convertHeaders(routesManifest.headers)); } + if (routesManifest.pages404) { + hasPages404 = true; + } + if (routesManifest.basePath && routesManifest.basePath !== '/') { nextBasePath = routesManifest.basePath; @@ -512,6 +518,7 @@ export const build = async ({ const staticPages: { [key: string]: FileFsRef } = {}; const dynamicPages: string[] = []; const dynamicDataRoutes: Array = []; + let static404Page: string | undefined; const appMountPrefixNoTrailingSlash = path.posix .join('/', entryDirectory) @@ -637,6 +644,7 @@ export const build = async ({ } const staticRoute = path.join(entryDirectory, pathname); + staticPages[staticRoute] = staticPageFiles[page]; staticPages[staticRoute].contentType = htmlContentType; @@ -646,9 +654,18 @@ export const build = async ({ } }); - const pageKeys = Object.keys(pages); + // this can be either 404.html in latest versions + // or _errors/404.html versions while this was experimental + static404Page = + staticPages['404'] && hasPages404 + ? '404' + : staticPages['_errors/404'] + ? '_errors/404' + : undefined; + // > 1 because _error is a lambda but isn't used if a static 404 is available - const hasLambdas = !staticPages['_errors/404'] || pageKeys.length > 1; + const pageKeys = Object.keys(pages); + const hasLambdas = !static404Page || pageKeys.length > 1; if (pageKeys.length === 0) { const nextConfig = await getNextConfig(workPath, entryPath); @@ -794,8 +811,13 @@ export const build = async ({ return; } - // Don't create _error lambda if we have a static 404 page - if (staticPages['_errors/404'] && page === '_error.js') { + // Don't create _error lambda if we have a static 404 page or + // pages404 is enabled and 404.js is present + if ( + page === '_error.js' && + ((static404Page && staticPages[static404Page]) || + (hasPages404 && pages['404.js'])) + ) { return; } @@ -824,10 +846,10 @@ export const build = async ({ config, }); + const outputName = path.join(entryDirectory, pathname); + if (requiresTracing) { - lambdas[ - path.join(entryDirectory, pathname) - ] = await createLambdaFromPseudoLayers({ + lambdas[outputName] = await createLambdaFromPseudoLayers({ files: { ...launcherFiles, [requiresTracing ? pageFileName : 'page.js']: pages[page], @@ -838,7 +860,7 @@ export const build = async ({ ...lambdaOptions, }); } else { - lambdas[path.join(entryDirectory, pathname)] = await createLambda({ + lambdas[outputName] = await createLambda({ files: { ...launcherFiles, ...assets, @@ -1084,7 +1106,13 @@ export const build = async ({ dest: path.join( '/', entryDirectory, - staticPages['_errors/404'] ? '_errors/404' : '_error' + static404Page + ? static404Page + : // if static 404 is not present but we have pages/404.js + // it is a lambda due to _app getInitialProps + hasPages404 && lambdas['404'] + ? '404' + : '_error' ), status: 404, }, diff --git a/packages/now-next/src/utils.ts b/packages/now-next/src/utils.ts index 77c1711322a..c566bd5f4ad 100644 --- a/packages/now-next/src/utils.ts +++ b/packages/now-next/src/utils.ts @@ -306,6 +306,7 @@ type RoutesManifestRegex = { }; export type RoutesManifest = { + pages404: boolean; basePath: string | undefined; redirects: (Redirect & RoutesManifestRegex)[]; rewrites: (NowRewrite & RoutesManifestRegex)[]; diff --git a/packages/now-next/test/fixtures/05-spr-support/package.json b/packages/now-next/test/fixtures/05-spr-support/package.json index d368bc213b6..2b54f9f9bfe 100644 --- a/packages/now-next/test/fixtures/05-spr-support/package.json +++ b/packages/now-next/test/fixtures/05-spr-support/package.json @@ -1,6 +1,6 @@ { "dependencies": { - "next": "^9.1.6-canary.1", + "next": "9.1.6-canary.1", "react": "^16.8.6", "react-dom": "^16.8.6" } diff --git a/packages/now-next/test/fixtures/19-pages-404/next.config.js b/packages/now-next/test/fixtures/19-pages-404/next.config.js new file mode 100644 index 00000000000..ac717504266 --- /dev/null +++ b/packages/now-next/test/fixtures/19-pages-404/next.config.js @@ -0,0 +1,5 @@ +module.exports = { + generateBuildId() { + return 'testing-build-id'; + }, +}; diff --git a/packages/now-next/test/fixtures/19-pages-404/now.json b/packages/now-next/test/fixtures/19-pages-404/now.json new file mode 100644 index 00000000000..6c837e96a2a --- /dev/null +++ b/packages/now-next/test/fixtures/19-pages-404/now.json @@ -0,0 +1,24 @@ +{ + "version": 2, + "builds": [{ "src": "package.json", "use": "@now/next" }], + "probes": [ + { + "path": "/", + "mustContain": "Hi" + }, + { + "path": "/", + "responseHeaders": { + "x-now-cache": "HIT" + } + }, + { + "path": "/non-existent", + "mustContain": "custom 404!!" + }, + { + "path": "/non-existent", + "mustContain": "__next" + } + ] +} diff --git a/packages/now-next/test/fixtures/19-pages-404/package.json b/packages/now-next/test/fixtures/19-pages-404/package.json new file mode 100644 index 00000000000..6f8622e202e --- /dev/null +++ b/packages/now-next/test/fixtures/19-pages-404/package.json @@ -0,0 +1,7 @@ +{ + "dependencies": { + "next": "9.2.3-canary.4", + "react": "^16.8.6", + "react-dom": "^16.8.6" + } +} diff --git a/packages/now-next/test/fixtures/19-pages-404/pages/404.js b/packages/now-next/test/fixtures/19-pages-404/pages/404.js new file mode 100644 index 00000000000..2e3b2e62081 --- /dev/null +++ b/packages/now-next/test/fixtures/19-pages-404/pages/404.js @@ -0,0 +1 @@ +export default () => 'custom 404!!'; diff --git a/packages/now-next/test/fixtures/19-pages-404/pages/index.js b/packages/now-next/test/fixtures/19-pages-404/pages/index.js new file mode 100644 index 00000000000..cdeee3e6335 --- /dev/null +++ b/packages/now-next/test/fixtures/19-pages-404/pages/index.js @@ -0,0 +1 @@ +export default () => 'Hi'; diff --git a/packages/now-next/test/fixtures/20-pages-404-lambda/next.config.js b/packages/now-next/test/fixtures/20-pages-404-lambda/next.config.js new file mode 100644 index 00000000000..ac717504266 --- /dev/null +++ b/packages/now-next/test/fixtures/20-pages-404-lambda/next.config.js @@ -0,0 +1,5 @@ +module.exports = { + generateBuildId() { + return 'testing-build-id'; + }, +}; diff --git a/packages/now-next/test/fixtures/20-pages-404-lambda/now.json b/packages/now-next/test/fixtures/20-pages-404-lambda/now.json new file mode 100644 index 00000000000..913f4d777d4 --- /dev/null +++ b/packages/now-next/test/fixtures/20-pages-404-lambda/now.json @@ -0,0 +1,18 @@ +{ + "version": 2, + "builds": [{ "src": "package.json", "use": "@now/next" }], + "probes": [ + { + "path": "/", + "mustContain": "Hi" + }, + { + "path": "/non-existent", + "mustContain": "custom 404!!" + }, + { + "path": "/non-existent", + "mustContain": "__next" + } + ] +} diff --git a/packages/now-next/test/fixtures/20-pages-404-lambda/package.json b/packages/now-next/test/fixtures/20-pages-404-lambda/package.json new file mode 100644 index 00000000000..6f8622e202e --- /dev/null +++ b/packages/now-next/test/fixtures/20-pages-404-lambda/package.json @@ -0,0 +1,7 @@ +{ + "dependencies": { + "next": "9.2.3-canary.4", + "react": "^16.8.6", + "react-dom": "^16.8.6" + } +} diff --git a/packages/now-next/test/fixtures/20-pages-404-lambda/pages/404.js b/packages/now-next/test/fixtures/20-pages-404-lambda/pages/404.js new file mode 100644 index 00000000000..2e3b2e62081 --- /dev/null +++ b/packages/now-next/test/fixtures/20-pages-404-lambda/pages/404.js @@ -0,0 +1 @@ +export default () => 'custom 404!!'; diff --git a/packages/now-next/test/fixtures/20-pages-404-lambda/pages/_app.js b/packages/now-next/test/fixtures/20-pages-404-lambda/pages/_app.js new file mode 100644 index 00000000000..22962239412 --- /dev/null +++ b/packages/now-next/test/fixtures/20-pages-404-lambda/pages/_app.js @@ -0,0 +1,5 @@ +const App = ({ Component, pageProps }) => ; + +App.getInitialProps = () => ({ hello: 'world' }); + +export default App; diff --git a/packages/now-next/test/fixtures/20-pages-404-lambda/pages/index.js b/packages/now-next/test/fixtures/20-pages-404-lambda/pages/index.js new file mode 100644 index 00000000000..cdeee3e6335 --- /dev/null +++ b/packages/now-next/test/fixtures/20-pages-404-lambda/pages/index.js @@ -0,0 +1 @@ +export default () => 'Hi';