Warn when user has pages/_error but no pages/404 (#11603)

* Warn when user has pages/_error but no pages/404 * Update test * Update custom-error-no-custom-404.md * Apply suggestions from code review Co-Authored-By: Tim Neutkens <[email protected]> Co-authored-by: Tim Neutkens <[email protected]>
vercel · Apr 5, 2020 · 1cdc607 · 1cdc607
1 parent 9baa103
commit 1cdc607
Show file tree

Hide file tree

Showing 5 changed files with 92 additions and 24 deletions.
diff --git a/errors/custom-error-no-custom-404.md b/errors/custom-error-no-custom-404.md
@@ -0,0 +1,14 @@
+# Custom /\_error without /404
+
+#### Why This Error Occurred
+
+You added a custom `/_error` page without adding a custom `/404` page. Adding a custom `/_error` typically opts your application out of having the automatic static optimization applied to the 404 page.
+
+#### Possible Ways to Fix It
+
+Add a `pages/404.js` with the 404 page you would like to show.
+
+### Useful Links
+
+- [Automatic Static Optimization](https://nextjs.org/docs/advanced-features/automatic-static-optimization)
+- [404 Page](https://nextjs.org/docs/advanced-features/custom-error-page#404-page)
diff --git a/packages/next/build/utils.ts b/packages/next/build/utils.ts
@@ -96,13 +96,13 @@ export async function printTreeView(
   ]
 
   const hasCustomApp = await findPageFile(pagesDir, '/_app', pageExtensions)
-  const hasCustomError = await findPageFile(pagesDir, '/_error', pageExtensions)
 
-  if (useStatic404) {
-    pageInfos.set('/404', {
-      ...(pageInfos.get('/404') || pageInfos.get('/_error')),
-      static: true,
-    } as any)
+  pageInfos.set('/404', {
+    ...(pageInfos.get('/404') || pageInfos.get('/_error')),
+    static: useStatic404,
+  } as any)
+
+  if (!list.includes('/404')) {
     list = [...list, '/404']
   }
 
@@ -120,8 +120,8 @@ export async function printTreeView(
       e =>
         !(
           e === '/_document' ||
-          (!hasCustomApp && e === '/_app') ||
-          (!hasCustomError && e === '/_error')
+          e === '/_error' ||
+          (!hasCustomApp && e === '/_app')
         )
     )
     .sort((a, b) => a.localeCompare(b))

diff --git a/packages/next/next-server/server/next-server.ts b/packages/next/next-server/server/next-server.ts
@@ -1,5 +1,6 @@
 import compression from 'next/dist/compiled/compression'
 import fs from 'fs'
+import chalk from 'next/dist/compiled/chalk'
 import { IncomingMessage, ServerResponse } from 'http'
 import Proxy from 'next/dist/compiled/http-proxy'
 import nanoid from 'next/dist/compiled/nanoid/index.js'
@@ -60,6 +61,7 @@ import {
   initializeSprCache,
   setSprCache,
 } from './spr-cache'
+import { execOnce } from '../lib/utils'
 import { isBlockedPage } from './utils'
 import { loadEnvConfig } from '../../lib/load-env-config'
 
@@ -1262,6 +1264,15 @@ export default class Server {
     return this.sendHTML(req, res, html)
   }
 
+  private customErrorNo404Warn = execOnce(() => {
+    console.warn(
+      chalk.bold.yellow(`Warning: `) +
+        chalk.yellow(
+          `You have added a custom /_error page without a custom /404 page. This prevents the 404 page from being auto statically optimized.\nSee here for info: https://err.sh/next.js/custom-error-no-custom-404`
+        )
+    )
+  })
+
   public async renderErrorToHTML(
     err: Error | null,
     req: IncomingMessage,
@@ -1284,6 +1295,14 @@ export default class Server {
       result = await this.findPageComponents('/_error', query)
     }
 
+    if (
+      process.env.NODE_ENV !== 'production' &&
+      !using404Page &&
+      (await this.hasPage('/_error'))
+    ) {
+      this.customErrorNo404Warn()
+    }
+
     let html: string | null
     try {
       try {

diff --git a/test/integration/build-output/test/index.test.js b/test/integration/build-output/test/index.test.js
@@ -103,7 +103,7 @@ describe('Build Output', () => {
       })
 
       expect(stdout).toMatch(/\/ [ ]* \d{1,} B/)
-      expect(stdout).toMatch(/λ \/_error [ ]* \d{1,} B/)
+      expect(stdout).toMatch(/λ \/404 [ ]* \d{1,} B/)
       expect(stdout).toMatch(/\+ First Load JS shared by all [ 0-9.]* kB/)
       expect(stdout).toMatch(/ runtime\/main\.[0-9a-z]{6}\.js [ 0-9.]* kB/)
       expect(stdout).toMatch(/ chunks\/framework\.[0-9a-z]{6}\.js [ 0-9. ]* kB/)
@@ -112,7 +112,7 @@ describe('Build Output', () => {
       expect(stdout).not.toContain(' /_app')
       expect(stdout).not.toContain('<buildId>')
 
-      expect(stdout).toContain(' /_error')
+      expect(stdout).not.toContain(' /_error')
       expect(stdout).toContain('○ /')
     })
   })
@@ -124,22 +124,12 @@ describe('Build Output', () => {
       await remove(join(appDir, '.next'))
     })
 
-    // FIXME: this should be static
-    xit('should specify /_error as static', async () => {
-      const { stdout } = await nextBuild(appDir, [], {
-        stdout: true,
-      })
-      expect(stdout).toContain('○ /_error')
-      expect(stdout).not.toContain('<buildId>')
-    })
-
-    // This test is not really correct.
-    // Remove this when fixed and enable the above one.
-    it('should specify /_error as lambda even when static', async () => {
+    it('should not specify /404 as lambda when static', async () => {
       const { stdout } = await nextBuild(appDir, [], {
         stdout: true,
       })
-      expect(stdout).toContain('λ /_error')
+      expect(stdout).toContain('○ /404')
+      expect(stdout).not.toContain('λ /_error')
       expect(stdout).not.toContain('<buildId>')
     })
   })

diff --git a/test/integration/custom-error/test/index.test.js b/test/integration/custom-error/test/index.test.js
@@ -8,6 +8,7 @@ import {
   nextBuild,
   nextStart,
   killApp,
+  launchApp,
 } from 'next-test-utils'
 
 jasmine.DEFAULT_TIMEOUT_INTERVAL = 1000 * 60 * 2
@@ -23,15 +24,59 @@ const runTests = () => {
   })
 }
 
+const customErrNo404Match = /You have added a custom \/_error page without a custom \/404 page/
+
 describe('Custom _error', () => {
+  describe('dev mode', () => {
+    let stderr = ''
+
+    beforeAll(async () => {
+      appPort = await findPort()
+      app = await launchApp(appDir, appPort, {
+        onStderr(msg) {
+          stderr += msg || ''
+        },
+      })
+    })
+    afterAll(() => killApp())
+
+    it('should not warn with /_error and /404', async () => {
+      stderr = ''
+      const page404 = join(appDir, 'pages/404.js')
+      await fs.writeFile(page404, `export default () => 'not found...'`)
+      const html = await renderViaHTTP(appPort, '/404')
+      await fs.remove(page404)
+      expect(html).toContain('not found...')
+      expect(stderr).not.toMatch(customErrNo404Match)
+    })
+
+    it('should warn on custom /_error without custom /404', async () => {
+      stderr = ''
+      const html = await renderViaHTTP(appPort, '/404')
+      expect(html).toContain('An error 404 occurred on server')
+      expect(stderr).toMatch(customErrNo404Match)
+    })
+  })
+
   describe('production mode', () => {
+    let buildOutput = ''
+
     beforeAll(async () => {
-      await nextBuild(appDir)
+      const { stdout, stderr } = await nextBuild(appDir, undefined, {
+        stdout: true,
+        stderr: true,
+      })
+      buildOutput = (stdout || '') + (stderr || '')
       appPort = await findPort()
       app = await nextStart(appDir, appPort)
     })
     afterAll(() => killApp(app))
 
+    it('should not contain /_error in build output', async () => {
+      expect(buildOutput).toMatch(/λ .*?\/404/)
+      expect(buildOutput).not.toMatch(/λ .*?\/_error/)
+    })
+
     runTests()
   })