[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: vercel/next.js#10329 x-ref: vercel/next.js#10593
vercel · Feb 19, 2020 · 5be2917 · 5be2917
1 parent 5f08b24
commit 5be2917
Show file tree

Hide file tree

Showing 14 changed files with 114 additions and 10 deletions.
diff --git 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<Source> = [];
+  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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -0,0 +1,5 @@
+const App = ({ Component, pageProps }) => <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
@@ -0,0 +1 @@
+export default () => 'Hi';