From db90ffe1ea7fe4c3dde86fce8e45470c803a7063 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Sat, 1 Feb 2020 08:47:42 -0600 Subject: [PATCH] Implement experimental pages/404.js for custom 404 page (#10329) * Implement experimental pages/404.js for custom 404 page * Make sure to show error for getInitialProps in pages/404 in dev mode also * Update routes-manifest tests for new value * Make sure page404 is boolean in routes-manifest * Rename variables for consistency * Make sure to only use 404 page for 404 error --- errors/404-get-initial-props.md | 15 ++ packages/next/build/index.ts | 28 ++- packages/next/lib/constants.ts | 2 + packages/next/next-server/server/config.ts | 1 + .../next/next-server/server/next-server.ts | 40 ++- packages/next/next-server/server/render.tsx | 7 + packages/next/server/next-dev-server.ts | 10 + test/integration/404-page/next.config.js | 5 + test/integration/404-page/pages/404.js | 2 + test/integration/404-page/pages/err.js | 5 + test/integration/404-page/pages/index.js | 1 + test/integration/404-page/test/index.test.js | 233 ++++++++++++++++++ .../custom-routes/test/index.test.js | 1 + .../dynamic-routing/test/index.test.js | 1 + 14 files changed, 340 insertions(+), 11 deletions(-) create mode 100644 errors/404-get-initial-props.md create mode 100644 test/integration/404-page/next.config.js create mode 100644 test/integration/404-page/pages/404.js create mode 100644 test/integration/404-page/pages/err.js create mode 100644 test/integration/404-page/pages/index.js create mode 100644 test/integration/404-page/test/index.test.js diff --git a/errors/404-get-initial-props.md b/errors/404-get-initial-props.md new file mode 100644 index 000000000000000..dc0e1ecb3510c7f --- /dev/null +++ b/errors/404-get-initial-props.md @@ -0,0 +1,15 @@ +# 404.js Cannot Have getInitialProps + +#### Why This Error Occurred + +In your `404.js` page you added `getInitialProps` or `getServerProps` which is not allowed as this prevents the page from being static and `404.js` is meant to provide more flexibility for a static 404 page. Having a static 404 page is the most ideal as this page can be served very often and if not static puts extra strain on your server and more invocations for serverless functions which increase the cost of hosting your site unnecessarily. + +#### Possible Ways to Fix It + +Remove `getInitialProps` from `404.js` and make sure no HOC's used in `404.js` attach `getInitialProps`. + +If you want to fetch data for your `404.js` page move it to a client side fetch inside of `componentDidMount` or `useEffect(() => {}, [])`. + +### Useful Links + +- [Automatic Static Optimization](https://nextjs.org/docs/advanced-features/automatic-static-optimization) diff --git a/packages/next/build/index.ts b/packages/next/build/index.ts index 67cbdc381bf684e..f2ec3a9ce06e17f 100644 --- a/packages/next/build/index.ts +++ b/packages/next/build/index.ts @@ -16,7 +16,10 @@ import checkCustomRoutes, { Rewrite, Header, } from '../lib/check-custom-routes' -import { PUBLIC_DIR_MIDDLEWARE_CONFLICT } from '../lib/constants' +import { + PUBLIC_DIR_MIDDLEWARE_CONFLICT, + PAGES_404_GET_INITIAL_PROPS_ERROR, +} from '../lib/constants' import { findPagesDir } from '../lib/find-pages-dir' import { recursiveDelete } from '../lib/recursive-delete' import { recursiveReadDir } from '../lib/recursive-readdir' @@ -202,6 +205,10 @@ export default async function build(dir: string, conf = null): Promise { const hasCustomErrorPage = mappedPages['/_error'].startsWith( 'private-next-pages' ) + const hasPages404 = + config.experimental.pages404 && + mappedPages['/404'] && + mappedPages['/404'].startsWith('private-next-pages') if (hasPublicDir) { try { @@ -262,6 +269,7 @@ export default async function build(dir: string, conf = null): Promise { const routesManifestPath = path.join(distDir, ROUTES_MANIFEST) const routesManifest: any = { version: 1, + pages404: !!hasPages404, basePath: config.experimental.basePath, redirects: redirects.map(r => buildCustomRoute(r, 'redirect')), rewrites: rewrites.map(r => buildCustomRoute(r, 'rewrite')), @@ -511,6 +519,17 @@ export default async function build(dir: string, conf = null): Promise { staticPages.add(page) isStatic = true } + + if (hasPages404 && page === '/404') { + if (!result.isStatic) { + throw new Error(PAGES_404_GET_INITIAL_PROPS_ERROR) + } + // we need to ensure the 404 lambda is present since we use + // it when _app has getInitialProps + if (customAppGetInitialProps) { + staticPages.delete(page) + } + } } catch (err) { if (err.message !== 'INVALID_DEFAULT_EXPORT') throw err invalidPages.add(page) @@ -569,8 +588,7 @@ export default async function build(dir: string, conf = null): Promise { // Only export the static 404 when there is no /_error present const useStatic404 = !customAppGetInitialProps && - !hasCustomErrorPage && - config.experimental.static404 + ((!hasCustomErrorPage && config.experimental.static404) || hasPages404) if (invalidPages.size > 0) { throw new Error( @@ -640,7 +658,9 @@ export default async function build(dir: string, conf = null): Promise { }) if (useStatic404) { - defaultMap['/_errors/404'] = { page: '/_error' } + defaultMap['/_errors/404'] = { + page: hasPages404 ? '/404' : '/_error', + } } return defaultMap diff --git a/packages/next/lib/constants.ts b/packages/next/lib/constants.ts index 1bf62bb57e918f4..4391cbc755c04b2 100644 --- a/packages/next/lib/constants.ts +++ b/packages/next/lib/constants.ts @@ -29,3 +29,5 @@ export const SSG_GET_INITIAL_PROPS_CONFLICT = `You can not use getInitialProps w export const SERVER_PROPS_GET_INIT_PROPS_CONFLICT = `You can not use getInitialProps with unstable_getServerProps. Please remove one or the other` export const SERVER_PROPS_SSG_CONFLICT = `You can not use unstable_getStaticProps with unstable_getServerProps. To use SSG, please remove your unstable_getServerProps` + +export const PAGES_404_GET_INITIAL_PROPS_ERROR = `\`pages/404\` can not have getInitialProps/getServerProps, https://err.sh/zeit/next.js/404-get-initial-props` diff --git a/packages/next/next-server/server/config.ts b/packages/next/next-server/server/config.ts index ad975b8414d6471..4416fbd277dd667 100644 --- a/packages/next/next-server/server/config.ts +++ b/packages/next/next-server/server/config.ts @@ -53,6 +53,7 @@ const defaultConfig: { [key: string]: any } = { workerThreads: false, basePath: '', static404: true, + pages404: false, }, future: { excludeDefaultMomentLocales: false, diff --git a/packages/next/next-server/server/next-server.ts b/packages/next/next-server/server/next-server.ts index 451fdedf07b372c..a67d149c04a3bea 100644 --- a/packages/next/next-server/server/next-server.ts +++ b/packages/next/next-server/server/next-server.ts @@ -101,6 +101,7 @@ export default class Server { documentMiddlewareEnabled: boolean hasCssMode: boolean dev?: boolean + pages404?: boolean } private compression?: Middleware private onErrorMiddleware?: ({ err }: { err: Error }) => Promise @@ -148,6 +149,7 @@ export default class Server { staticMarkup, buildId: this.buildId, generateEtags, + pages404: this.nextConfig.experimental.pages404, } // Only the `publicRuntimeConfig` key is exposed to the client side @@ -857,6 +859,11 @@ export default class Server { result: LoadComponentsReturnType, opts: any ): Promise { + // we need to ensure the status code if /404 is visited directly + if (this.nextConfig.experimental.pages404 && pathname === '/404') { + res.statusCode = 404 + } + // handle static page if (typeof result.Component === 'string') { return result.Component @@ -1115,13 +1122,32 @@ export default class Server { ) { let result: null | LoadComponentsReturnType = null + const { static404, pages404 } = this.nextConfig.experimental + const is404 = res.statusCode === 404 + let using404Page = false + // use static 404 page if available and is 404 response - if (this.nextConfig.experimental.static404 && res.statusCode === 404) { - try { - result = await this.findPageComponents('/_errors/404') - } catch (err) { - if (err.code !== 'ENOENT') { - throw err + if (is404) { + if (static404) { + try { + result = await this.findPageComponents('/_errors/404') + } catch (err) { + if (err.code !== 'ENOENT') { + throw err + } + } + } + + // use 404 if /_errors/404 isn't available which occurs + // during development and when _app has getInitialProps + if (!result && pages404) { + try { + result = await this.findPageComponents('/404') + using404Page = true + } catch (err) { + if (err.code !== 'ENOENT') { + throw err + } } } } @@ -1135,7 +1161,7 @@ export default class Server { html = await this.renderToHTMLWithComponents( req, res, - '/_error', + using404Page ? '/404' : '/_error', query, result, { diff --git a/packages/next/next-server/server/render.tsx b/packages/next/next-server/server/render.tsx index cbc72b44a7cbca5..50eaad0e69ab273 100644 --- a/packages/next/next-server/server/render.tsx +++ b/packages/next/next-server/server/render.tsx @@ -28,6 +28,7 @@ import { SSG_GET_INITIAL_PROPS_CONFLICT, SERVER_PROPS_GET_INIT_PROPS_CONFLICT, SERVER_PROPS_SSG_CONFLICT, + PAGES_404_GET_INITIAL_PROPS_ERROR, } from '../../lib/constants' import { AMP_RENDER_TARGET } from '../lib/constants' import { LoadComponentsReturnType, ManifestItem } from './load-components' @@ -135,6 +136,7 @@ type RenderOpts = LoadComponentsReturnType & { documentMiddlewareEnabled?: boolean isDataReq?: boolean params?: ParsedUrlQuery + pages404?: boolean } function renderDocument( @@ -263,6 +265,7 @@ export async function renderToHTML( unstable_getServerProps, isDataReq, params, + pages404, } = renderOpts const callMiddleware = async (method: string, args: any[], props = false) => { @@ -363,6 +366,10 @@ export async function renderToHTML( req.url = pathname renderOpts.nextExport = true } + + if (pages404 && pathname === '/404' && !isAutoExport) { + throw new Error(PAGES_404_GET_INITIAL_PROPS_ERROR) + } } if (isAutoExport) renderOpts.autoExport = true if (isSpr) renderOpts.nextExport = false diff --git a/packages/next/server/next-dev-server.ts b/packages/next/server/next-dev-server.ts index c2a30ecf61e3a3b..248b38f80acbe17 100644 --- a/packages/next/server/next-dev-server.ts +++ b/packages/next/server/next-dev-server.ts @@ -432,6 +432,16 @@ export default class DevServer extends Server { }) } catch (err) { if (err.code === 'ENOENT') { + if (this.nextConfig.experimental.pages404) { + try { + await this.hotReloader!.ensurePage('/404') + } catch (err) { + if (err.code !== 'ENOENT') { + throw err + } + } + } + res.statusCode = 404 return this.renderErrorToHTML(null, req, res, pathname, query) } diff --git a/test/integration/404-page/next.config.js b/test/integration/404-page/next.config.js new file mode 100644 index 000000000000000..5a9fb106c667a05 --- /dev/null +++ b/test/integration/404-page/next.config.js @@ -0,0 +1,5 @@ +module.exports = { + experimental: { + pages404: true, + }, +} diff --git a/test/integration/404-page/pages/404.js b/test/integration/404-page/pages/404.js new file mode 100644 index 000000000000000..e5e684d08cdc958 --- /dev/null +++ b/test/integration/404-page/pages/404.js @@ -0,0 +1,2 @@ +const page = () => 'custom 404 page' +export default page diff --git a/test/integration/404-page/pages/err.js b/test/integration/404-page/pages/err.js new file mode 100644 index 000000000000000..938b5e49ac2963f --- /dev/null +++ b/test/integration/404-page/pages/err.js @@ -0,0 +1,5 @@ +const page = () => 'custom 404 page' +page.getInitialProps = () => { + throw new Error('oops') +} +export default page diff --git a/test/integration/404-page/pages/index.js b/test/integration/404-page/pages/index.js new file mode 100644 index 000000000000000..f6c15d1f66e8a6d --- /dev/null +++ b/test/integration/404-page/pages/index.js @@ -0,0 +1 @@ +export default () => 'hello from index' diff --git a/test/integration/404-page/test/index.test.js b/test/integration/404-page/test/index.test.js new file mode 100644 index 000000000000000..e6c28cee3871ac8 --- /dev/null +++ b/test/integration/404-page/test/index.test.js @@ -0,0 +1,233 @@ +/* eslint-env jest */ +/* global jasmine */ +import fs from 'fs-extra' +import { join } from 'path' +import { + killApp, + findPort, + launchApp, + nextStart, + nextBuild, + renderViaHTTP, + fetchViaHTTP, + waitFor, +} from 'next-test-utils' + +jasmine.DEFAULT_TIMEOUT_INTERVAL = 1000 * 60 * 2 + +const appDir = join(__dirname, '../') +const pages404 = join(appDir, 'pages/404.js') +const appPage = join(appDir, 'pages/_app.js') +const nextConfig = join(appDir, 'next.config.js') + +let nextConfigContent +let buildId +let appPort +let app + +const runTests = (mode = 'server') => { + it('should use pages/404', async () => { + const html = await renderViaHTTP(appPort, '/abc') + expect(html).toContain('custom 404 page') + }) + + it('should set correct status code with pages/404', async () => { + const res = await fetchViaHTTP(appPort, '/abc') + expect(res.status).toBe(404) + }) + + it('should not error when visited directly', async () => { + const res = await fetchViaHTTP(appPort, '/404') + expect(res.status).toBe(404) + expect(await res.text()).toContain('custom 404 page') + }) + + it('should render _error for a 500 error still', async () => { + const html = await renderViaHTTP(appPort, '/err') + expect(html).not.toContain('custom 404 page') + expect(html).toContain(mode === 'dev' ? 'oops' : 'Internal Server Error') + }) + + if (mode !== 'dev') { + it('should output _errors/404.html during build', async () => { + expect( + await fs.exists( + join( + appDir, + '.next', + mode === 'serverless' + ? 'serverless/pages' + : `server/static/${buildId}/pages`, + '_errors/404.html' + ) + ) + ).toBe(true) + }) + + it('should add _errors/404 to pages-manifest correctly', async () => { + const manifest = await fs.readJSON( + join(appDir, '.next', mode, 'pages-manifest.json') + ) + expect('/_errors/404' in manifest).toBe(true) + }) + } +} + +describe('404 Page Support', () => { + describe('dev mode', () => { + beforeAll(async () => { + appPort = await findPort() + app = await launchApp(appDir, appPort) + buildId = 'development' + }) + afterAll(() => killApp(app)) + + runTests('dev') + }) + + describe('server mode', () => { + beforeAll(async () => { + await nextBuild(appDir) + appPort = await findPort() + app = await nextStart(appDir, appPort) + buildId = await fs.readFile(join(appDir, '.next/BUILD_ID'), 'utf8') + }) + afterAll(() => killApp(app)) + + runTests('server') + }) + + describe('serverless mode', () => { + beforeAll(async () => { + nextConfigContent = await fs.readFile(nextConfig, 'utf8') + await fs.writeFile( + nextConfig, + ` + module.exports = { + target: 'serverless', + experimental: { + pages404: true + } + } + ` + ) + await nextBuild(appDir) + appPort = await findPort() + app = await nextStart(appDir, appPort) + buildId = await fs.readFile(join(appDir, '.next/BUILD_ID'), 'utf8') + }) + afterAll(async () => { + await fs.writeFile(nextConfig, nextConfigContent) + await killApp(app) + }) + + runTests('serverless') + }) + + it('falls back to _error correctly without pages/404', async () => { + await fs.move(pages404, `${pages404}.bak`) + appPort = await findPort() + app = await launchApp(appDir, appPort) + const res = await fetchViaHTTP(appPort, '/abc') + + await fs.move(`${pages404}.bak`, pages404) + await killApp(app) + + expect(res.status).toBe(404) + expect(await res.text()).toContain('This page could not be found') + }) + + it('shows error with getInitialProps in pages/404 build', async () => { + await fs.move(pages404, `${pages404}.bak`) + await fs.writeFile( + pages404, + ` + const page = () => 'custom 404 page' + page.getInitialProps = () => ({ a: 'b' }) + export default page + ` + ) + const { stderr, code } = await nextBuild(appDir, [], { stderr: true }) + await fs.remove(pages404) + await fs.move(`${pages404}.bak`, pages404) + + expect(stderr).toContain( + `\`pages/404\` can not have getInitialProps/getServerProps, https://err.sh/zeit/next.js/404-get-initial-props` + ) + expect(code).toBe(1) + }) + + it('shows error with getInitialProps in pages/404 dev', async () => { + await fs.move(pages404, `${pages404}.bak`) + await fs.writeFile( + pages404, + ` + const page = () => 'custom 404 page' + page.getInitialProps = () => ({ a: 'b' }) + export default page + ` + ) + + let stderr = '' + appPort = await findPort() + app = await launchApp(appDir, appPort, { + onStderr(msg) { + stderr += msg || '' + }, + }) + await renderViaHTTP(appPort, '/abc') + await waitFor(1000) + + await killApp(app) + + await fs.remove(pages404) + await fs.move(`${pages404}.bak`, pages404) + + const error = `\`pages/404\` can not have getInitialProps/getServerProps, https://err.sh/zeit/next.js/404-get-initial-props` + + expect(stderr).toContain(error) + }) + + describe('_app with getInitialProps', () => { + beforeAll(async () => { + await fs.writeFile( + appPage, + ` + import NextApp from 'next/app' + const App = ({ Component, pageProps }) => + App.getInitialProps = NextApp.getInitialProps + export default App + ` + ) + await nextBuild(appDir) + appPort = await findPort() + app = await nextStart(appDir, appPort) + buildId = await fs.readFile(join(appDir, '.next/BUILD_ID'), 'utf8') + }) + afterAll(async () => { + await fs.remove(appPage) + await killApp(app) + }) + + it('should not output static 404 if _app has getInitialProps', async () => { + expect( + await fs.exists( + join(appDir, '.next/server/static', buildId, 'pages/_errors/404.html') + ) + ).toBe(false) + }) + + it('specify to use the 404 page still in the routes-manifest', async () => { + const manifest = await fs.readJSON( + join(appDir, '.next/routes-manifest.json') + ) + expect(manifest.pages404).toBe(true) + }) + + it('should still use 404 page', async () => { + const res = await fetchViaHTTP(appPort, '/abc') + expect(res.status).toBe(404) + expect(await res.text()).toContain('custom 404 page') + }) + }) +}) diff --git a/test/integration/custom-routes/test/index.test.js b/test/integration/custom-routes/test/index.test.js index 5b954b48bd91a91..f2c01f903adba0b 100644 --- a/test/integration/custom-routes/test/index.test.js +++ b/test/integration/custom-routes/test/index.test.js @@ -293,6 +293,7 @@ const runTests = (isDev = false) => { expect(manifest).toEqual({ version: 1, + pages404: false, basePath: '', redirects: [ { diff --git a/test/integration/dynamic-routing/test/index.test.js b/test/integration/dynamic-routing/test/index.test.js index 7796b18f86c3d74..756f47232367685 100644 --- a/test/integration/dynamic-routing/test/index.test.js +++ b/test/integration/dynamic-routing/test/index.test.js @@ -431,6 +431,7 @@ function runTests(dev) { expect(manifest).toEqual({ version: 1, + pages404: false, basePath: '', headers: [], rewrites: [],