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

packages/next/client/page-loader.ts

@@ -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)
-              }
-
-              // 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 */
-                })
-              }
+          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}"]`
+                  )
+                ) {
+                  // 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[]) => {

test/integration/client-404/pages/missing.js

+export default function Missing() {
+  return <p id="missing">poof</p>
+}

test/integration/client-404/pages/to-missing-link.js

+import Link from 'next/link'
+
+export default () => (
+  <Link href="/missing">
+    <a id="to-missing">to 404</a>
+  </Link>
+)

test/integration/client-404/test/client-navigation.js

 /* 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/)
+      })
+    }
   })
 }

test/integration/client-404/test/index.test.js

@@ -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)
   })
 })