From bb4aca6475f2e9908288afeae320683fcb82391d Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Wed, 29 Jan 2020 15:25:08 -0600 Subject: [PATCH 1/6] Implement experimental pages/404.js for custom 404 page --- errors/404-get-initial-props.md | 15 ++ packages/next/build/index.ts | 23 ++- packages/next/next-server/server/config.ts | 1 + .../next/next-server/server/next-server.ts | 25 ++- 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/index.js | 1 + test/integration/404-page/test/index.test.js | 195 ++++++++++++++++++ 9 files changed, 272 insertions(+), 5 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/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 000000000000..dc0e1ecb3510 --- /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 67cbdc381bf6..bb1248ef45ac 100644 --- a/packages/next/build/index.ts +++ b/packages/next/build/index.ts @@ -202,6 +202,10 @@ export default async function build(dir: string, conf = null): Promise { const hasCustomErrorPage = mappedPages['/_error'].startsWith( 'private-next-pages' ) + const hasPage404 = + config.experimental.pages404 && + mappedPages['/404'] && + mappedPages['/404'].startsWith('private-next-pages') if (hasPublicDir) { try { @@ -262,6 +266,7 @@ export default async function build(dir: string, conf = null): Promise { const routesManifestPath = path.join(distDir, ROUTES_MANIFEST) const routesManifest: any = { version: 1, + page404: hasPage404, basePath: config.experimental.basePath, redirects: redirects.map(r => buildCustomRoute(r, 'redirect')), rewrites: rewrites.map(r => buildCustomRoute(r, 'rewrite')), @@ -511,6 +516,19 @@ export default async function build(dir: string, conf = null): Promise { staticPages.add(page) isStatic = true } + + if (hasPage404 && page === '/404') { + if (!result.isStatic) { + throw new Error( + `\`pages/404\` can not have getInitialProps/getServerProps, https://err.sh/zeit/next.js/404-get-initial-props` + ) + } + // 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 +587,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) || hasPage404) if (invalidPages.size > 0) { throw new Error( @@ -640,7 +657,7 @@ export default async function build(dir: string, conf = null): Promise { }) if (useStatic404) { - defaultMap['/_errors/404'] = { page: '/_error' } + defaultMap['/_errors/404'] = { page: hasPage404 ? '/404' : '/_error' } } return defaultMap diff --git a/packages/next/next-server/server/config.ts b/packages/next/next-server/server/config.ts index eee2087ff1f0..780247a910c1 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 062e51422cf4..ee73e1414b9d 100644 --- a/packages/next/next-server/server/next-server.ts +++ b/packages/next/next-server/server/next-server.ts @@ -857,6 +857,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,8 +1120,11 @@ export default class Server { ) { let result: null | LoadComponentsReturnType = null + const { static404, pages404 } = this.nextConfig.experimental + let using404Page = false + // use static 404 page if available and is 404 response - if (this.nextConfig.experimental.static404 && res.statusCode === 404) { + if (static404 && res.statusCode === 404) { try { result = await this.findPageComponents('/_errors/404') } catch (err) { @@ -1126,6 +1134,19 @@ export default class Server { } } + // 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 + } + } + } + if (!result) { result = await this.findPageComponents('/_error', query) } @@ -1135,7 +1156,7 @@ export default class Server { html = await this.renderToHTMLWithComponents( req, res, - '/_error', + using404Page ? '/404' : '/_error', query, result, { diff --git a/packages/next/server/next-dev-server.ts b/packages/next/server/next-dev-server.ts index c2a30ecf61e3..248b38f80acb 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 000000000000..5a9fb106c667 --- /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 000000000000..e5e684d08cdc --- /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/index.js b/test/integration/404-page/pages/index.js new file mode 100644 index 000000000000..f6c15d1f66e8 --- /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 000000000000..23b916ec85dd --- /dev/null +++ b/test/integration/404-page/test/index.test.js @@ -0,0 +1,195 @@ +/* eslint-env jest */ +/* global jasmine */ +import fs from 'fs-extra' +import { join } from 'path' +import { + killApp, + findPort, + launchApp, + nextStart, + nextBuild, + renderViaHTTP, + fetchViaHTTP, +} from 'next-test-utils' + +jasmine.DEFAULT_TIMEOUT_INTERVAL = 1000 * 60 * 2 + +const appDir = join(__dirname, '../') +const page404 = 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') + }) + + 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(page404, `${page404}.bak`) + appPort = await findPort() + app = await launchApp(appDir, appPort) + const res = await fetchViaHTTP(appPort, '/abc') + + await fs.move(`${page404}.bak`, page404) + 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', async () => { + await fs.move(page404, `${page404}.bak`) + await fs.writeFile( + page404, + ` + const page = () => 'custom 404 page' + page.getInitialProps = () => ({ a: 'b' }) + export default page + ` + ) + const { stderr, code } = await nextBuild(appDir, [], { stderr: true }) + await fs.remove(page404) + await fs.move(`${page404}.bak`, page404) + + expect(stderr).toContain( + `\`pages/404\` can not have getInitialProps/getServerProps, https://err.sh/zeit/next.js/404-get-initial-props` + ) + expect(code).toBe(1) + }) + + 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.page404).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') + }) + }) +}) From 3ede95e142c339613d6e6a73e84c31e82d207baf Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Wed, 29 Jan 2020 15:38:27 -0600 Subject: [PATCH 2/6] Make sure to show error for getInitialProps in pages/404 in dev mode also --- packages/next/build/index.ts | 9 ++--- packages/next/lib/constants.ts | 2 ++ .../next/next-server/server/next-server.ts | 2 ++ packages/next/next-server/server/render.tsx | 7 ++++ test/integration/404-page/test/index.test.js | 34 ++++++++++++++++++- 5 files changed, 49 insertions(+), 5 deletions(-) diff --git a/packages/next/build/index.ts b/packages/next/build/index.ts index bb1248ef45ac..5da01ed1f693 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' @@ -519,9 +522,7 @@ export default async function build(dir: string, conf = null): Promise { if (hasPage404 && page === '/404') { if (!result.isStatic) { - throw new Error( - `\`pages/404\` can not have getInitialProps/getServerProps, https://err.sh/zeit/next.js/404-get-initial-props` - ) + 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 diff --git a/packages/next/lib/constants.ts b/packages/next/lib/constants.ts index 1bf62bb57e91..4391cbc755c0 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/next-server.ts b/packages/next/next-server/server/next-server.ts index ee73e1414b9d..8c81cd18a8e9 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 diff --git a/packages/next/next-server/server/render.tsx b/packages/next/next-server/server/render.tsx index cbc72b44a7cb..50eaad0e69ab 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/test/integration/404-page/test/index.test.js b/test/integration/404-page/test/index.test.js index 23b916ec85dd..45dc928367de 100644 --- a/test/integration/404-page/test/index.test.js +++ b/test/integration/404-page/test/index.test.js @@ -10,6 +10,7 @@ import { nextBuild, renderViaHTTP, fetchViaHTTP, + waitFor, } from 'next-test-utils' jasmine.DEFAULT_TIMEOUT_INTERVAL = 1000 * 60 * 2 @@ -130,7 +131,7 @@ describe('404 Page Support', () => { expect(await res.text()).toContain('This page could not be found') }) - it('shows error with getInitialProps in pages/404', async () => { + it('shows error with getInitialProps in pages/404 build', async () => { await fs.move(page404, `${page404}.bak`) await fs.writeFile( page404, @@ -150,6 +151,37 @@ describe('404 Page Support', () => { expect(code).toBe(1) }) + it('shows error with getInitialProps in pages/404 dev', async () => { + await fs.move(page404, `${page404}.bak`) + await fs.writeFile( + page404, + ` + 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(page404) + await fs.move(`${page404}.bak`, page404) + + 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( From 713386091d286fc5f6ac921242d8ed86c78502b7 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Wed, 29 Jan 2020 15:50:50 -0600 Subject: [PATCH 3/6] Update routes-manifest tests for new value --- test/integration/custom-routes/test/index.test.js | 1 + test/integration/dynamic-routing/test/index.test.js | 1 + 2 files changed, 2 insertions(+) diff --git a/test/integration/custom-routes/test/index.test.js b/test/integration/custom-routes/test/index.test.js index 5b954b48bd91..995302b6d88d 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, + page404: false, basePath: '', redirects: [ { diff --git a/test/integration/dynamic-routing/test/index.test.js b/test/integration/dynamic-routing/test/index.test.js index 4edbcae89075..5b5e65c0e2bc 100644 --- a/test/integration/dynamic-routing/test/index.test.js +++ b/test/integration/dynamic-routing/test/index.test.js @@ -419,6 +419,7 @@ function runTests(dev) { expect(manifest).toEqual({ version: 1, + page404: false, basePath: '', headers: [], rewrites: [], From 8cd7fa72f90227d269a06d9db07904d2f0b1e18a Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Wed, 29 Jan 2020 15:59:54 -0600 Subject: [PATCH 4/6] Make sure page404 is boolean in routes-manifest --- packages/next/build/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/next/build/index.ts b/packages/next/build/index.ts index 5da01ed1f693..8496338df5ef 100644 --- a/packages/next/build/index.ts +++ b/packages/next/build/index.ts @@ -269,7 +269,7 @@ export default async function build(dir: string, conf = null): Promise { const routesManifestPath = path.join(distDir, ROUTES_MANIFEST) const routesManifest: any = { version: 1, - page404: hasPage404, + page404: !!hasPage404, basePath: config.experimental.basePath, redirects: redirects.map(r => buildCustomRoute(r, 'redirect')), rewrites: rewrites.map(r => buildCustomRoute(r, 'rewrite')), From 39c385e880fce5662c21ba418078c2f363f5f677 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Wed, 29 Jan 2020 16:38:42 -0600 Subject: [PATCH 5/6] Rename variables for consistency --- packages/next/build/index.ts | 12 ++++++---- test/integration/404-page/test/index.test.js | 24 +++++++++---------- .../custom-routes/test/index.test.js | 2 +- .../dynamic-routing/test/index.test.js | 2 +- 4 files changed, 21 insertions(+), 19 deletions(-) diff --git a/packages/next/build/index.ts b/packages/next/build/index.ts index 8496338df5ef..f2ec3a9ce06e 100644 --- a/packages/next/build/index.ts +++ b/packages/next/build/index.ts @@ -205,7 +205,7 @@ export default async function build(dir: string, conf = null): Promise { const hasCustomErrorPage = mappedPages['/_error'].startsWith( 'private-next-pages' ) - const hasPage404 = + const hasPages404 = config.experimental.pages404 && mappedPages['/404'] && mappedPages['/404'].startsWith('private-next-pages') @@ -269,7 +269,7 @@ export default async function build(dir: string, conf = null): Promise { const routesManifestPath = path.join(distDir, ROUTES_MANIFEST) const routesManifest: any = { version: 1, - page404: !!hasPage404, + pages404: !!hasPages404, basePath: config.experimental.basePath, redirects: redirects.map(r => buildCustomRoute(r, 'redirect')), rewrites: rewrites.map(r => buildCustomRoute(r, 'rewrite')), @@ -520,7 +520,7 @@ export default async function build(dir: string, conf = null): Promise { isStatic = true } - if (hasPage404 && page === '/404') { + if (hasPages404 && page === '/404') { if (!result.isStatic) { throw new Error(PAGES_404_GET_INITIAL_PROPS_ERROR) } @@ -588,7 +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) || hasPage404) + ((!hasCustomErrorPage && config.experimental.static404) || hasPages404) if (invalidPages.size > 0) { throw new Error( @@ -658,7 +658,9 @@ export default async function build(dir: string, conf = null): Promise { }) if (useStatic404) { - defaultMap['/_errors/404'] = { page: hasPage404 ? '/404' : '/_error' } + defaultMap['/_errors/404'] = { + page: hasPages404 ? '/404' : '/_error', + } } return defaultMap diff --git a/test/integration/404-page/test/index.test.js b/test/integration/404-page/test/index.test.js index 45dc928367de..5208f99d9b08 100644 --- a/test/integration/404-page/test/index.test.js +++ b/test/integration/404-page/test/index.test.js @@ -16,7 +16,7 @@ import { jasmine.DEFAULT_TIMEOUT_INTERVAL = 1000 * 60 * 2 const appDir = join(__dirname, '../') -const page404 = join(appDir, 'pages/404.js') +const pages404 = join(appDir, 'pages/404.js') const appPage = join(appDir, 'pages/_app.js') const nextConfig = join(appDir, 'next.config.js') @@ -119,12 +119,12 @@ describe('404 Page Support', () => { }) it('falls back to _error correctly without pages/404', async () => { - await fs.move(page404, `${page404}.bak`) + await fs.move(pages404, `${pages404}.bak`) appPort = await findPort() app = await launchApp(appDir, appPort) const res = await fetchViaHTTP(appPort, '/abc') - await fs.move(`${page404}.bak`, page404) + await fs.move(`${pages404}.bak`, pages404) await killApp(app) expect(res.status).toBe(404) @@ -132,9 +132,9 @@ describe('404 Page Support', () => { }) it('shows error with getInitialProps in pages/404 build', async () => { - await fs.move(page404, `${page404}.bak`) + await fs.move(pages404, `${pages404}.bak`) await fs.writeFile( - page404, + pages404, ` const page = () => 'custom 404 page' page.getInitialProps = () => ({ a: 'b' }) @@ -142,8 +142,8 @@ describe('404 Page Support', () => { ` ) const { stderr, code } = await nextBuild(appDir, [], { stderr: true }) - await fs.remove(page404) - await fs.move(`${page404}.bak`, page404) + 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` @@ -152,9 +152,9 @@ describe('404 Page Support', () => { }) it('shows error with getInitialProps in pages/404 dev', async () => { - await fs.move(page404, `${page404}.bak`) + await fs.move(pages404, `${pages404}.bak`) await fs.writeFile( - page404, + pages404, ` const page = () => 'custom 404 page' page.getInitialProps = () => ({ a: 'b' }) @@ -174,8 +174,8 @@ describe('404 Page Support', () => { await killApp(app) - await fs.remove(page404) - await fs.move(`${page404}.bak`, page404) + 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` @@ -215,7 +215,7 @@ describe('404 Page Support', () => { const manifest = await fs.readJSON( join(appDir, '.next/routes-manifest.json') ) - expect(manifest.page404).toBe(true) + expect(manifest.pages404).toBe(true) }) it('should still use 404 page', async () => { diff --git a/test/integration/custom-routes/test/index.test.js b/test/integration/custom-routes/test/index.test.js index 995302b6d88d..f2c01f903adb 100644 --- a/test/integration/custom-routes/test/index.test.js +++ b/test/integration/custom-routes/test/index.test.js @@ -293,7 +293,7 @@ const runTests = (isDev = false) => { expect(manifest).toEqual({ version: 1, - page404: false, + 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 5b5e65c0e2bc..c7df30fa67c9 100644 --- a/test/integration/dynamic-routing/test/index.test.js +++ b/test/integration/dynamic-routing/test/index.test.js @@ -419,7 +419,7 @@ function runTests(dev) { expect(manifest).toEqual({ version: 1, - page404: false, + pages404: false, basePath: '', headers: [], rewrites: [], From 499cd63435412f4286766574c7975825efa97c42 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Wed, 29 Jan 2020 18:58:54 -0600 Subject: [PATCH 6/6] Make sure to only use 404 page for 404 error --- .../next/next-server/server/next-server.ts | 35 ++++++++++--------- test/integration/404-page/pages/err.js | 5 +++ test/integration/404-page/test/index.test.js | 6 ++++ 3 files changed, 30 insertions(+), 16 deletions(-) create mode 100644 test/integration/404-page/pages/err.js diff --git a/packages/next/next-server/server/next-server.ts b/packages/next/next-server/server/next-server.ts index 8c81cd18a8e9..2026e6f61320 100644 --- a/packages/next/next-server/server/next-server.ts +++ b/packages/next/next-server/server/next-server.ts @@ -1123,28 +1123,31 @@ 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 (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 + // 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 + } } } } diff --git a/test/integration/404-page/pages/err.js b/test/integration/404-page/pages/err.js new file mode 100644 index 000000000000..938b5e49ac29 --- /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/test/index.test.js b/test/integration/404-page/test/index.test.js index 5208f99d9b08..e6c28cee3871 100644 --- a/test/integration/404-page/test/index.test.js +++ b/test/integration/404-page/test/index.test.js @@ -42,6 +42,12 @@ const runTests = (mode = 'server') => { 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(