Make loadPage
track success of script loading (#16334)
Prior to this PR, `loadPage` would call `loadScript` which would then report if the script failed to load. This was problematic because `loadScript` notified a failure to load via `pageRegisterEvents`, which would not set the `pageCache` value for future requests. This means a one-off promise rejection would happen, [in lieu of being] typically consumed within the client-side router, causing a server-side reload. However, when `loadPage` was used independently (i.e. to preload pages), this promise rejection would be ignored as a preload failure. When the real routing request comes in, the `loadPage` function skips its attempt to load the `<script>` because it was already in the DOM, and the router would stop functioning. --- To fix this behavior, I've removed erroneous emits on `pageRegisterEvents` to only happen during the page registration lifecycle (its intended use). The new behavior is that `loadScript` returns a `Promise` that `loadPage` can track, and if any of the page(s) scripts fail to load, we mark the entire page as errored in `pageCache`. This ensures future requests to `loadPage` will always immediately reject with a `PAGE_LOAD_ERROR`, which causes the server-side redirect at the appropriate point. --- Fixes #16333
This commit is contained in:
parent
8ef253fd02
commit
0a72d14deb
5 changed files with 107 additions and 48 deletions
|
@ -80,6 +80,20 @@ function appendLink(href: string, rel: string, as?: string): Promise<any> {
|
|||
return res
|
||||
}
|
||||
|
||||
function loadScript(url: string): Promise<any> {
|
||||
return new Promise((res, rej) => {
|
||||
const script = document.createElement('script')
|
||||
if (process.env.__NEXT_MODERN_BUILD && hasNoModule) {
|
||||
script.type = 'module'
|
||||
}
|
||||
script.crossOrigin = process.env.__NEXT_CROSS_ORIGIN!
|
||||
script.src = url
|
||||
script.onload = res
|
||||
script.onerror = () => rej(pageLoadError(url))
|
||||
document.body.appendChild(script)
|
||||
})
|
||||
}
|
||||
|
||||
export type GoodPageCache = {
|
||||
page: ComponentType
|
||||
mod: any
|
||||
|
@ -173,14 +187,11 @@ export default class PageLoader {
|
|||
}
|
||||
|
||||
// Returns a promise for the dependencies for a particular route
|
||||
getDependencies(route: string): Promise<string[]> {
|
||||
private getDependencies(route: string): Promise<string[]> {
|
||||
return this.promisedBuildManifest!.then((m) => {
|
||||
return m[route]
|
||||
? m[route].map((url) => `${this.assetPrefix}/_next/${encodeURI(url)}`)
|
||||
: (this.pageRegisterEvents.emit(route, {
|
||||
error: pageLoadError(route),
|
||||
}),
|
||||
[])
|
||||
: Promise.reject(pageLoadError(route))
|
||||
})
|
||||
}
|
||||
|
||||
|
@ -311,32 +322,45 @@ export default class PageLoader {
|
|||
if (!this.loadingRoutes[route]) {
|
||||
this.loadingRoutes[route] = true
|
||||
if (process.env.NODE_ENV === 'production') {
|
||||
this.getDependencies(route).then((deps) => {
|
||||
deps.forEach((d) => {
|
||||
if (
|
||||
d.endsWith('.js') &&
|
||||
!document.querySelector(`script[src^="${d}"]`)
|
||||
) {
|
||||
this.loadScript(d, route)
|
||||
}
|
||||
this.getDependencies(route)
|
||||
.then((deps) => {
|
||||
const pending: Promise<any>[] = []
|
||||
deps.forEach((d) => {
|
||||
if (
|
||||
d.endsWith('.js') &&
|
||||
!document.querySelector(`script[src^="${d}"]`)
|
||||
) {
|
||||
pending.push(loadScript(d))
|
||||
}
|
||||
|
||||
// Prefetch CSS as it'll be needed when the page JavaScript
|
||||
// evaluates. This will only trigger if explicit prefetching is
|
||||
// disabled for a <Link>... prefetching in this case is desirable
|
||||
// because we *know* it's going to be used very soon (page was
|
||||
// loaded).
|
||||
if (
|
||||
d.endsWith('.css') &&
|
||||
!document.querySelector(
|
||||
`link[rel="${relPreload}"][href^="${d}"]`
|
||||
)
|
||||
) {
|
||||
appendLink(d, relPreload, 'style').catch(() => {
|
||||
/* ignore preload error */
|
||||
})
|
||||
}
|
||||
// Prefetch CSS as it'll be needed when the page JavaScript
|
||||
// evaluates. This will only trigger if explicit prefetching is
|
||||
// disabled for a <Link>... prefetching in this case is desirable
|
||||
// because we *know* it's going to be used very soon (page was
|
||||
// loaded).
|
||||
if (
|
||||
d.endsWith('.css') &&
|
||||
!document.querySelector(
|
||||
`link[rel="${relPreload}"][href^="${d}"]`
|
||||
)
|
||||
) {
|
||||
// This is not pushed into `pending` because we don't need to
|
||||
// wait for these to resolve. To prevent an unhandled
|
||||
// rejection, we swallow the error which is handled later in
|
||||
// the rendering cycle (this is just a preload optimization).
|
||||
appendLink(d, relPreload, 'style').catch(() => {
|
||||
/* ignore preload error */
|
||||
})
|
||||
}
|
||||
})
|
||||
return Promise.all(pending)
|
||||
})
|
||||
.catch((err) => {
|
||||
// Mark the page as failed to load if any of its required scripts
|
||||
// fail to load:
|
||||
this.pageCache[route] = { error: err }
|
||||
fire({ error: err })
|
||||
})
|
||||
})
|
||||
} else {
|
||||
// Development only. In production the page file is part of the build manifest
|
||||
route = normalizeRoute(route)
|
||||
|
@ -345,25 +369,16 @@ export default class PageLoader {
|
|||
const url = `${this.assetPrefix}/_next/static/chunks/pages${encodeURI(
|
||||
scriptRoute
|
||||
)}`
|
||||
this.loadScript(url, route)
|
||||
loadScript(url).catch((err) => {
|
||||
// Mark the page as failed to load if its script fails to load:
|
||||
this.pageCache[route] = { error: err }
|
||||
fire({ error: err })
|
||||
})
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
loadScript(url: string, route: string) {
|
||||
const script = document.createElement('script')
|
||||
if (process.env.__NEXT_MODERN_BUILD && hasNoModule) {
|
||||
script.type = 'module'
|
||||
}
|
||||
script.crossOrigin = process.env.__NEXT_CROSS_ORIGIN!
|
||||
script.src = url
|
||||
script.onerror = () => {
|
||||
this.pageRegisterEvents.emit(route, { error: pageLoadError(url) })
|
||||
}
|
||||
document.body.appendChild(script)
|
||||
}
|
||||
|
||||
// This method if called by the route code.
|
||||
registerPage(route: string, regFn: () => any) {
|
||||
const register = (styleSheets: string[]) => {
|
||||
|
|
3
test/integration/client-404/pages/missing.js
Normal file
3
test/integration/client-404/pages/missing.js
Normal file
|
@ -0,0 +1,3 @@
|
|||
export default function Missing() {
|
||||
return <p id="missing">poof</p>
|
||||
}
|
7
test/integration/client-404/pages/to-missing-link.js
Normal file
7
test/integration/client-404/pages/to-missing-link.js
Normal file
|
@ -0,0 +1,7 @@
|
|||
import Link from 'next/link'
|
||||
|
||||
export default () => (
|
||||
<Link href="/missing">
|
||||
<a id="to-missing">to 404</a>
|
||||
</Link>
|
||||
)
|
|
@ -1,8 +1,8 @@
|
|||
/* eslint-env jest */
|
||||
import webdriver from 'next-webdriver'
|
||||
import { check } from 'next-test-utils'
|
||||
import { check, waitFor } from 'next-test-utils'
|
||||
|
||||
export default (context) => {
|
||||
export default (context, isProd = false) => {
|
||||
describe('Client Navigation 404', () => {
|
||||
describe('should show 404 upon client replacestate', () => {
|
||||
it('should navigate the page', async () => {
|
||||
|
@ -31,5 +31,28 @@ export default (context) => {
|
|||
await check(() => browser.elementByCss('#errorStatusCode').text(), /404/)
|
||||
expect(await browser.eval(() => window.beforeNav)).not.toBe('hi')
|
||||
})
|
||||
|
||||
if (isProd) {
|
||||
it('should hard navigate to URL on failing to load missing bundle', async () => {
|
||||
const browser = await webdriver(context.appPort, '/to-missing-link')
|
||||
await browser.eval(() => (window.beforeNav = 'hi'))
|
||||
expect(
|
||||
await browser.eval(() =>
|
||||
document.querySelector('script[src*="pages/missing"]')
|
||||
)
|
||||
).toBeFalsy()
|
||||
await browser.elementByCss('#to-missing').moveTo()
|
||||
await waitFor(2000)
|
||||
expect(
|
||||
await browser.eval(() =>
|
||||
document.querySelector('script[src*="pages/missing"]')
|
||||
)
|
||||
).toBeTruthy()
|
||||
await browser.elementByCss('#to-missing').click()
|
||||
await waitFor(2000)
|
||||
expect(await browser.eval(() => window.beforeNav)).not.toBe('hi')
|
||||
await check(() => browser.elementByCss('#missing').text(), /poof/)
|
||||
})
|
||||
}
|
||||
})
|
||||
}
|
||||
|
|
|
@ -8,7 +8,9 @@ import {
|
|||
killApp,
|
||||
nextBuild,
|
||||
nextStart,
|
||||
getBuildManifest,
|
||||
} from 'next-test-utils'
|
||||
import fs from 'fs-extra'
|
||||
|
||||
// test suite
|
||||
import clientNavigation from './client-navigation'
|
||||
|
@ -17,8 +19,8 @@ const context = {}
|
|||
const appDir = join(__dirname, '../')
|
||||
jest.setTimeout(1000 * 60 * 5)
|
||||
|
||||
const runTests = () => {
|
||||
clientNavigation(context, (p, q) => renderViaHTTP(context.appPort, p, q))
|
||||
const runTests = (isProd = false) => {
|
||||
clientNavigation(context, isProd)
|
||||
}
|
||||
|
||||
describe('Client 404', () => {
|
||||
|
@ -40,9 +42,18 @@ describe('Client 404', () => {
|
|||
await nextBuild(appDir)
|
||||
context.appPort = await findPort()
|
||||
context.server = await nextStart(appDir, context.appPort)
|
||||
|
||||
const manifest = await getBuildManifest(appDir)
|
||||
const files = manifest.pages['/missing'].filter((d) =>
|
||||
/static[\\/]chunks[\\/]pages/.test(d)
|
||||
)
|
||||
if (files.length < 1) {
|
||||
throw new Error('oops!')
|
||||
}
|
||||
await Promise.all(files.map((f) => fs.remove(join(appDir, '.next', f))))
|
||||
})
|
||||
afterAll(() => killApp(context.server))
|
||||
|
||||
runTests()
|
||||
runTests(true)
|
||||
})
|
||||
})
|
||||
|
|
Loading…
Reference in a new issue