From 3cd45bc33120fce5c9a34857e9ae4a1f825ab38b Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Thu, 20 Feb 2020 15:25:41 -0600 Subject: [PATCH] Make sure to handle failing to load _error (#10617) --- .../next/next-server/lib/router/router.ts | 111 ++++++++++-------- .../error-load-fail/pages/broken.js | 7 ++ .../error-load-fail/pages/index.js | 9 ++ .../error-load-fail/test/index.test.js | 36 ++++++ 4 files changed, 111 insertions(+), 52 deletions(-) create mode 100644 test/integration/error-load-fail/pages/broken.js create mode 100644 test/integration/error-load-fail/pages/index.js create mode 100644 test/integration/error-load-fail/test/index.test.js diff --git a/packages/next/next-server/lib/router/router.ts b/packages/next/next-server/lib/router/router.ts index 83caa11e55278f1..ad79afe701a3e2a 100644 --- a/packages/next/next-server/lib/router/router.ts +++ b/packages/next/next-server/lib/router/router.ts @@ -500,6 +500,64 @@ export default class Router implements BaseRouter { return Promise.resolve(cachedRouteInfo) } + const handleError = ( + err: Error & { code: any; cancelled: boolean }, + loadErrorFail?: boolean + ) => { + return new Promise(resolve => { + if (err.code === 'PAGE_LOAD_ERROR' || loadErrorFail) { + // If we can't load the page it could be one of following reasons + // 1. Page doesn't exists + // 2. Page does exist in a different zone + // 3. Internal error while loading the page + + // So, doing a hard reload is the proper way to deal with this. + window.location.href = as + + // Changing the URL doesn't block executing the current code path. + // So, we need to mark it as a cancelled error and stop the routing logic. + err.cancelled = true + // @ts-ignore TODO: fix the control flow here + return resolve({ error: err }) + } + + if (err.cancelled) { + // @ts-ignore TODO: fix the control flow here + return resolve({ error: err }) + } + + resolve( + this.fetchComponent('/_error') + .then(Component => { + const routeInfo: RouteInfo = { Component, err } + return new Promise(resolve => { + this.getInitialProps(Component, { + err, + pathname, + query, + } as any).then( + props => { + routeInfo.props = props + routeInfo.error = err + resolve(routeInfo) + }, + gipErr => { + console.error( + 'Error in error page `getInitialProps`: ', + gipErr + ) + routeInfo.error = err + routeInfo.props = {} + resolve(routeInfo) + } + ) + }) as Promise + }) + .catch(err => handleError(err, true)) + ) + }) as Promise + } + return (new Promise((resolve, reject) => { if (cachedRouteInfo) { return resolve(cachedRouteInfo) @@ -542,58 +600,7 @@ export default class Router implements BaseRouter { return routeInfo }) }) - .catch(err => { - return new Promise(resolve => { - if (err.code === 'PAGE_LOAD_ERROR') { - // If we can't load the page it could be one of following reasons - // 1. Page doesn't exists - // 2. Page does exist in a different zone - // 3. Internal error while loading the page - - // So, doing a hard reload is the proper way to deal with this. - window.location.href = as - - // Changing the URL doesn't block executing the current code path. - // So, we need to mark it as a cancelled error and stop the routing logic. - err.cancelled = true - // @ts-ignore TODO: fix the control flow here - return resolve({ error: err }) - } - - if (err.cancelled) { - // @ts-ignore TODO: fix the control flow here - return resolve({ error: err }) - } - - resolve( - this.fetchComponent('/_error').then(Component => { - const routeInfo: RouteInfo = { Component, err } - return new Promise(resolve => { - this.getInitialProps(Component, { - err, - pathname, - query, - } as any).then( - props => { - routeInfo.props = props - routeInfo.error = err - resolve(routeInfo) - }, - gipErr => { - console.error( - 'Error in error page `getInitialProps`: ', - gipErr - ) - routeInfo.error = err - routeInfo.props = {} - resolve(routeInfo) - } - ) - }) as Promise - }) - ) - }) as Promise - }) + .catch(handleError) } set( diff --git a/test/integration/error-load-fail/pages/broken.js b/test/integration/error-load-fail/pages/broken.js new file mode 100644 index 000000000000000..fd070d6f1f7f2f3 --- /dev/null +++ b/test/integration/error-load-fail/pages/broken.js @@ -0,0 +1,7 @@ +const Page = () => 'oops' + +Page.getInitialProps = () => { + throw new Error('oops') +} + +export default Page diff --git a/test/integration/error-load-fail/pages/index.js b/test/integration/error-load-fail/pages/index.js new file mode 100644 index 000000000000000..95c50d4edaeb549 --- /dev/null +++ b/test/integration/error-load-fail/pages/index.js @@ -0,0 +1,9 @@ +import Link from 'next/link' + +export default () => ( + <> + + to oops + + +) diff --git a/test/integration/error-load-fail/test/index.test.js b/test/integration/error-load-fail/test/index.test.js new file mode 100644 index 000000000000000..fafdc88ff9d7172 --- /dev/null +++ b/test/integration/error-load-fail/test/index.test.js @@ -0,0 +1,36 @@ +/* eslint-env jest */ +/* global jasmine */ +import path from 'path' +import webdriver from 'next-webdriver' +import { + nextBuild, + nextStart, + findPort, + killApp, + waitFor, +} from 'next-test-utils' + +jasmine.DEFAULT_TIMEOUT_INTERVAL = 1000 * 60 * 1 +const appDir = path.join(__dirname, '..') + +describe('Failing to load _error', () => { + it('handles failing to load _error correctly', async () => { + await nextBuild(appDir) + const appPort = await findPort() + const app = await nextStart(appDir, appPort) + + const browser = await webdriver(appPort, '/') + await browser.eval(`window.beforeNavigate = true`) + + await browser.elementByCss('#to-broken').moveTo() + await browser.waitForElementByCss('script[src*="broken.js"') + + // stop app so that _error can't be loaded + await killApp(app) + + await browser.elementByCss('#to-broken').click() + await waitFor(2000) + + expect(await browser.eval('window.beforeNavigate')).toBeFalsy() + }) +})