Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make loadPage track success of script loading #16334

Merged
merged 2 commits into from
Aug 19, 2020

Conversation

Timer
Copy link
Member

@Timer Timer commented Aug 19, 2020

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

@ijjk
Copy link
Member

ijjk commented Aug 19, 2020

Stats from current PR

Default Server Mode (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary Timer/next.js refactor/page-loader Change
buildDuration 12.3s 12.3s -26ms
nodeModulesSize 57.7 MB 57.7 MB ⚠️ +1.73 kB
Page Load Tests Overall increase ✓
vercel/next.js canary Timer/next.js refactor/page-loader Change
/ failed reqs 0 0
/ total time (seconds) 2.163 2.229 ⚠️ +0.07
/ avg req/sec 1155.66 1121.52 ⚠️ -34.14
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.216 1.185 -0.03
/error-in-render avg req/sec 2055.45 2108.88 +53.43
Client Bundles (main, webpack, commons) Overall increase ⚠️
vercel/next.js canary Timer/next.js refactor/page-loader Change
677f882d2ed8..4e55.js gzip 10.2 kB 10.2 kB
framework.HASH.js gzip 39 kB 39 kB
main-8da9cf5..55dc.js gzip 7.18 kB 7.21 kB ⚠️ +30 B
webpack-e067..f178.js gzip 751 B 751 B
Overall change 57.1 kB 57.2 kB ⚠️ +30 B
Client Bundles (main, webpack, commons) Modern Overall increase ⚠️
vercel/next.js canary Timer/next.js refactor/page-loader Change
677f882d2ed8..dule.js gzip 6.11 kB 6.11 kB
framework.HA..dule.js gzip 39 kB 39 kB
main-e458620..dule.js gzip 6.2 kB 6.21 kB ⚠️ +10 B
webpack-07c5..dule.js gzip 751 B 751 B
Overall change 52 kB 52 kB ⚠️ +10 B
Legacy Client Bundles (polyfills)
vercel/next.js canary Timer/next.js refactor/page-loader Change
polyfills-4b..e242.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary Timer/next.js refactor/page-loader Change
_app-9a0b9e1..b37e.js gzip 1.28 kB 1.28 kB
_error-1464c..a26f.js gzip 3.44 kB 3.44 kB
hooks-89731c..c609.js gzip 887 B 887 B
index-17468f..5d83.js gzip 227 B 227 B
link-000f151..65d4.js gzip 1.29 kB 1.29 kB
routerDirect..924c.js gzip 284 B 284 B
withRouter-7..c13d.js gzip 284 B 284 B
Overall change 7.69 kB 7.69 kB
Client Pages Modern
vercel/next.js canary Timer/next.js refactor/page-loader Change
_app-75d3a82..dule.js gzip 625 B 625 B
_error-e550f..dule.js gzip 2.29 kB 2.29 kB
hooks-cbf13f..dule.js gzip 387 B 387 B
index-b9a643..dule.js gzip 226 B 226 B
link-4cfda7a..dule.js gzip 1.26 kB 1.26 kB
routerDirect..dule.js gzip 284 B 284 B
withRouter-f..dule.js gzip 282 B 282 B
Overall change 5.35 kB 5.35 kB
Client Build Manifests
vercel/next.js canary Timer/next.js refactor/page-loader Change
_buildManifest.js gzip 322 B 322 B
_buildManife..dule.js gzip 329 B 329 B
Overall change 651 B 651 B
Rendered Page Sizes Overall increase ⚠️
vercel/next.js canary Timer/next.js refactor/page-loader Change
index.html gzip 947 B 949 B ⚠️ +2 B
link.html gzip 953 B 953 B
withRouter.html gzip 938 B 940 B ⚠️ +2 B
Overall change 2.84 kB 2.84 kB ⚠️ +4 B

Diffs

Diff for main-02c0657..76.module.js
@@ -1313,6 +1313,24 @@
         return res;
       }
 
+      function loadScript(url) {
+        return new Promise((res, rej) => {
+          var script = document.createElement("script");
+
+          if (true && hasNoModule) {
+            script.type = "module";
+          }
+
+          script.crossOrigin = "anonymous";
+          script.src = url;
+          script.onload = res;
+
+          script.onerror = () => rej(pageLoadError(url));
+
+          document.body.appendChild(script);
+        });
+      }
+
       class PageLoader {
         constructor(buildId, assetPrefix, initialPage, initialStyleSheets) {
           this.initialPage = void 0;
@@ -1377,10 +1395,7 @@
               ? m[route].map(url =>
                   "".concat(this.assetPrefix, "/_next/").concat(encodeURI(url))
                 )
-              : (this.pageRegisterEvents.emit(route, {
-                  error: pageLoadError(route)
-                }),
-                []);
+              : Promise.reject(pageLoadError(route));
           });
         }
         /**
@@ -1514,57 +1529,55 @@
               this.loadingRoutes[route] = true;
 
               if (true) {
-                this.getDependencies(route).then(deps => {
-                  deps.forEach(d => {
-                    if (
-                      d.endsWith(".js") &&
-                      !document.querySelector('script[src^="'.concat(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="'
-                          .concat(relPreload, '"][href^="')
-                          .concat(d, '"]')
-                      )
-                    ) {
-                      appendLink(d, relPreload, "style").catch(() => {
-                        /* ignore preload error */
-                      });
-                    }
+                this.getDependencies(route)
+                  .then(deps => {
+                    var pending = [];
+                    deps.forEach(d => {
+                      if (
+                        d.endsWith(".js") &&
+                        !document.querySelector('script[src^="'.concat(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="'
+                            .concat(relPreload, '"][href^="')
+                            .concat(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 {
                 var url, scriptRoute;
               }
             }
           });
-        }
-
-        loadScript(url, route) {
-          var script = document.createElement("script");
-
-          if (true && hasNoModule) {
-            script.type = "module";
-          }
-
-          script.crossOrigin = "anonymous";
-          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, regFn) {
Diff for main-e9105a4..cb75348e7.js
@@ -1697,6 +1697,26 @@ _N_E = (window["webpackJsonp_N_E"] = window["webpackJsonp_N_E"] || []).push([
         return res;
       }
 
+      function loadScript(url) {
+        return new Promise(function(res, rej) {
+          var script = document.createElement("script");
+
+          if (true && hasNoModule) {
+            script.type = "module";
+          }
+
+          script.crossOrigin = "anonymous";
+          script.src = url;
+          script.onload = res;
+
+          script.onerror = function() {
+            return rej(pageLoadError(url));
+          };
+
+          document.body.appendChild(script);
+        });
+      }
+
       var PageLoader = /*#__PURE__*/ (function() {
         function PageLoader(
           buildId,
@@ -1777,10 +1797,7 @@ _N_E = (window["webpackJsonp_N_E"] = window["webpackJsonp_N_E"] || []).push([
                         .concat(_this.assetPrefix, "/_next/")
                         .concat(encodeURI(url));
                     })
-                  : (_this.pageRegisterEvents.emit(route, {
-                      error: pageLoadError(route)
-                    }),
-                    []);
+                  : Promise.reject(pageLoadError(route));
               });
             }
             /**
@@ -1944,71 +1961,66 @@ _N_E = (window["webpackJsonp_N_E"] = window["webpackJsonp_N_E"] || []).push([
                   _this4.loadingRoutes[route] = true;
 
                   if (true) {
-                    _this4.getDependencies(route).then(function(deps) {
-                      deps.forEach(function(d) {
-                        if (
-                          d.endsWith(".js") &&
-                          !document.querySelector(
-                            'script[src^="'.concat(d, '"]')
-                          )
-                        ) {
-                          _this4.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="'
-                              .concat(relPreload, '"][href^="')
-                              .concat(d, '"]')
-                          )
-                        ) {
-                          appendLink(d, relPreload, "style")["catch"](
-                            function() {
-                              /* ignore preload error */
-                            }
-                          );
-                        }
+                    _this4
+                      .getDependencies(route)
+                      .then(function(deps) {
+                        var pending = [];
+                        deps.forEach(function(d) {
+                          if (
+                            d.endsWith(".js") &&
+                            !document.querySelector(
+                              'script[src^="'.concat(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="'
+                                .concat(relPreload, '"][href^="')
+                                .concat(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"](
+                              function() {
+                                /* ignore preload error */
+                              }
+                            );
+                          }
+                        });
+                        return Promise.all(pending);
+                      })
+                      ["catch"](function(err) {
+                        // Mark the page as failed to load if any of its required scripts
+                        // fail to load:
+                        _this4.pageCache[route] = {
+                          error: err
+                        };
+                        fire({
+                          error: err
+                        });
                       });
-                    });
                   } else {
                     var url, scriptRoute;
                   }
                 }
               });
-            }
-          },
-          {
-            key: "loadScript",
-            value: function loadScript(url, route) {
-              var _this5 = this;
-
-              var script = document.createElement("script");
-
-              if (true && hasNoModule) {
-                script.type = "module";
-              }
-
-              script.crossOrigin = "anonymous";
-              script.src = url;
-
-              script.onerror = function() {
-                _this5.pageRegisterEvents.emit(route, {
-                  error: pageLoadError(url)
-                });
-              };
-
-              document.body.appendChild(script);
             } // This method if called by the route code.
           },
           {
             key: "registerPage",
             value: function registerPage(route, regFn) {
-              var _this6 = this;
+              var _this5 = this;
 
               var register = function register(styleSheets) {
                 try {
@@ -2018,15 +2030,15 @@ _N_E = (window["webpackJsonp_N_E"] = window["webpackJsonp_N_E"] || []).push([
                     mod: mod,
                     styleSheets: styleSheets
                   };
-                  _this6.pageCache[route] = pageData;
+                  _this5.pageCache[route] = pageData;
 
-                  _this6.pageRegisterEvents.emit(route, pageData);
+                  _this5.pageRegisterEvents.emit(route, pageData);
                 } catch (error) {
-                  _this6.pageCache[route] = {
+                  _this5.pageCache[route] = {
                     error: error
                   };
 
-                  _this6.pageRegisterEvents.emit(route, {
+                  _this5.pageRegisterEvents.emit(route, {
                     error: error
                   });
                 }
@@ -2052,11 +2064,11 @@ _N_E = (window["webpackJsonp_N_E"] = window["webpackJsonp_N_E"] || []).push([
                   );
                 },
                 function(error) {
-                  _this6.pageCache[route] = {
+                  _this5.pageCache[route] = {
                     error: error
                   };
 
-                  _this6.pageRegisterEvents.emit(route, {
+                  _this5.pageRegisterEvents.emit(route, {
                     error: error
                   });
                 }
@@ -2070,7 +2082,7 @@ _N_E = (window["webpackJsonp_N_E"] = window["webpackJsonp_N_E"] || []).push([
           {
             key: "prefetch",
             value: function prefetch(route, isDependency) {
-              var _this7 = this;
+              var _this6 = this;
 
               // https://github.com/GoogleChromeLabs/quicklink/blob/453a661fa1fa940e2d2e044452398e38c67a98fb/src/index.mjs#L115-L118
               // License: Apache 2.0
@@ -2112,7 +2124,7 @@ _N_E = (window["webpackJsonp_N_E"] = window["webpackJsonp_N_E"] || []).push([
                         this.getDependencies(route).then(function(urls) {
                           return Promise.all(
                             urls.map(function(dependencyUrl) {
-                              return _this7.prefetch(dependencyUrl, true);
+                              return _this6.prefetch(dependencyUrl, true);
                             })
                           );
                         })
Diff for index.html
@@ -6,7 +6,7 @@
     <meta name="next-head-count" content="2" />
     <link
       rel="preload"
-      href="/_next/static/chunks/main-02c06573f47461638176.module.js"
+      href="/_next/static/chunks/main-779f39f4fe96a9ebe825.module.js"
       as="script"
       crossorigin="anonymous"
     />
@@ -81,13 +81,13 @@
       src="/_next/static/chunks/polyfills-f73ba3fc145972ef83e9.js"
     ></script>
     <script
-      src="/_next/static/chunks/main-e9105a4c4bfcb75348e7.js"
+      src="/_next/static/chunks/main-f96ebf7814a35905d943.js"
       async=""
       crossorigin="anonymous"
       nomodule=""
     ></script>
     <script
-      src="/_next/static/chunks/main-02c06573f47461638176.module.js"
+      src="/_next/static/chunks/main-779f39f4fe96a9ebe825.module.js"
       async=""
       crossorigin="anonymous"
       type="module"
Diff for link.html
@@ -6,7 +6,7 @@
     <meta name="next-head-count" content="2" />
     <link
       rel="preload"
-      href="/_next/static/chunks/main-02c06573f47461638176.module.js"
+      href="/_next/static/chunks/main-779f39f4fe96a9ebe825.module.js"
       as="script"
       crossorigin="anonymous"
     />
@@ -86,13 +86,13 @@
       src="/_next/static/chunks/polyfills-f73ba3fc145972ef83e9.js"
     ></script>
     <script
-      src="/_next/static/chunks/main-e9105a4c4bfcb75348e7.js"
+      src="/_next/static/chunks/main-f96ebf7814a35905d943.js"
       async=""
       crossorigin="anonymous"
       nomodule=""
     ></script>
     <script
-      src="/_next/static/chunks/main-02c06573f47461638176.module.js"
+      src="/_next/static/chunks/main-779f39f4fe96a9ebe825.module.js"
       async=""
       crossorigin="anonymous"
       type="module"
Diff for withRouter.html
@@ -6,7 +6,7 @@
     <meta name="next-head-count" content="2" />
     <link
       rel="preload"
-      href="/_next/static/chunks/main-02c06573f47461638176.module.js"
+      href="/_next/static/chunks/main-779f39f4fe96a9ebe825.module.js"
       as="script"
       crossorigin="anonymous"
     />
@@ -81,13 +81,13 @@
       src="/_next/static/chunks/polyfills-f73ba3fc145972ef83e9.js"
     ></script>
     <script
-      src="/_next/static/chunks/main-e9105a4c4bfcb75348e7.js"
+      src="/_next/static/chunks/main-f96ebf7814a35905d943.js"
       async=""
       crossorigin="anonymous"
       nomodule=""
     ></script>
     <script
-      src="/_next/static/chunks/main-02c06573f47461638176.module.js"
+      src="/_next/static/chunks/main-779f39f4fe96a9ebe825.module.js"
       async=""
       crossorigin="anonymous"
       type="module"

Serverless Mode (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary Timer/next.js refactor/page-loader Change
buildDuration 13.7s 13.9s ⚠️ +197ms
nodeModulesSize 57.7 MB 57.7 MB ⚠️ +1.73 kB
Client Bundles (main, webpack, commons) Overall increase ⚠️
vercel/next.js canary Timer/next.js refactor/page-loader Change
677f882d2ed8..4e55.js gzip 10.2 kB 10.2 kB
framework.HASH.js gzip 39 kB 39 kB
main-8da9cf5..55dc.js gzip 7.18 kB N/A N/A
webpack-e067..f178.js gzip 751 B 751 B
main-92b2173..1cff.js gzip N/A 7.21 kB N/A
Overall change 57.1 kB 57.2 kB ⚠️ +30 B
Client Bundles (main, webpack, commons) Modern Overall increase ⚠️
vercel/next.js canary Timer/next.js refactor/page-loader Change
677f882d2ed8..dule.js gzip 6.11 kB 6.11 kB
framework.HA..dule.js gzip 39 kB 39 kB
main-e458620..dule.js gzip 6.2 kB N/A N/A
webpack-07c5..dule.js gzip 751 B 751 B
main-13b0492..dule.js gzip N/A 6.21 kB N/A
Overall change 52 kB 52 kB ⚠️ +10 B
Legacy Client Bundles (polyfills)
vercel/next.js canary Timer/next.js refactor/page-loader Change
polyfills-4b..e242.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary Timer/next.js refactor/page-loader Change
_app-9a0b9e1..b37e.js gzip 1.28 kB 1.28 kB
_error-1464c..a26f.js gzip 3.44 kB 3.44 kB
hooks-89731c..c609.js gzip 887 B 887 B
index-17468f..5d83.js gzip 227 B 227 B
link-000f151..65d4.js gzip 1.29 kB 1.29 kB
routerDirect..924c.js gzip 284 B 284 B
withRouter-7..c13d.js gzip 284 B 284 B
Overall change 7.69 kB 7.69 kB
Client Pages Modern
vercel/next.js canary Timer/next.js refactor/page-loader Change
_app-75d3a82..dule.js gzip 625 B 625 B
_error-e550f..dule.js gzip 2.29 kB 2.29 kB
hooks-cbf13f..dule.js gzip 387 B 387 B
index-b9a643..dule.js gzip 226 B 226 B
link-4cfda7a..dule.js gzip 1.26 kB 1.26 kB
routerDirect..dule.js gzip 284 B 284 B
withRouter-f..dule.js gzip 282 B 282 B
Overall change 5.35 kB 5.35 kB
Client Build Manifests
vercel/next.js canary Timer/next.js refactor/page-loader Change
_buildManifest.js gzip 322 B 322 B
_buildManife..dule.js gzip 329 B 329 B
Overall change 651 B 651 B
Serverless bundles
vercel/next.js canary Timer/next.js refactor/page-loader Change
_error.js 1.03 MB 1.03 MB
404.html 4.18 kB 4.18 kB
hooks.html 3.82 kB 3.82 kB
index.js 1.03 MB 1.03 MB
link.js 1.07 MB 1.07 MB
routerDirect.js 1.07 MB 1.07 MB
withRouter.js 1.07 MB 1.07 MB
Overall change 5.27 MB 5.27 MB
Commit: 8aeb4b0

@Timer Timer marked this pull request as ready for review August 19, 2020 06:38
@ijjk
Copy link
Member

ijjk commented Aug 19, 2020

Stats from current PR

Default Server Mode (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary Timer/next.js refactor/page-loader Change
buildDuration 10.4s 10.2s -123ms
nodeModulesSize 57.7 MB 57.7 MB ⚠️ +1.73 kB
Page Load Tests Overall decrease ⚠️
vercel/next.js canary Timer/next.js refactor/page-loader Change
/ failed reqs 0 0
/ total time (seconds) 1.894 1.861 -0.03
/ avg req/sec 1319.67 1343.69 +24.02
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.031 1.062 ⚠️ +0.03
/error-in-render avg req/sec 2423.83 2353.69 ⚠️ -70.14
Client Bundles (main, webpack, commons) Overall increase ⚠️
vercel/next.js canary Timer/next.js refactor/page-loader Change
677f882d2ed8..4e55.js gzip 10.2 kB 10.2 kB
framework.HASH.js gzip 39 kB 39 kB
main-ff9b23f..5a93.js gzip 7.17 kB 7.2 kB ⚠️ +30 B
webpack-e067..f178.js gzip 751 B 751 B
Overall change 57.1 kB 57.2 kB ⚠️ +30 B
Client Bundles (main, webpack, commons) Modern Overall increase ⚠️
vercel/next.js canary Timer/next.js refactor/page-loader Change
677f882d2ed8..dule.js gzip 6.11 kB 6.11 kB
framework.HA..dule.js gzip 39 kB 39 kB
main-b54ffe3..dule.js gzip 6.2 kB 6.21 kB ⚠️ +10 B
webpack-07c5..dule.js gzip 751 B 751 B
Overall change 52 kB 52 kB ⚠️ +10 B
Legacy Client Bundles (polyfills)
vercel/next.js canary Timer/next.js refactor/page-loader Change
polyfills-4b..e242.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary Timer/next.js refactor/page-loader Change
_app-9a0b9e1..b37e.js gzip 1.28 kB 1.28 kB
_error-1464c..a26f.js gzip 3.44 kB 3.44 kB
hooks-89731c..c609.js gzip 887 B 887 B
index-17468f..5d83.js gzip 227 B 227 B
link-000f151..65d4.js gzip 1.29 kB 1.29 kB
routerDirect..924c.js gzip 284 B 284 B
withRouter-7..c13d.js gzip 284 B 284 B
Overall change 7.69 kB 7.69 kB
Client Pages Modern
vercel/next.js canary Timer/next.js refactor/page-loader Change
_app-75d3a82..dule.js gzip 625 B 625 B
_error-e550f..dule.js gzip 2.29 kB 2.29 kB
hooks-cbf13f..dule.js gzip 387 B 387 B
index-b9a643..dule.js gzip 226 B 226 B
link-4cfda7a..dule.js gzip 1.26 kB 1.26 kB
routerDirect..dule.js gzip 284 B 284 B
withRouter-f..dule.js gzip 282 B 282 B
Overall change 5.35 kB 5.35 kB
Client Build Manifests
vercel/next.js canary Timer/next.js refactor/page-loader Change
_buildManifest.js gzip 322 B 322 B
_buildManife..dule.js gzip 329 B 329 B
Overall change 651 B 651 B
Rendered Page Sizes Overall decrease ✓
vercel/next.js canary Timer/next.js refactor/page-loader Change
index.html gzip 950 B 947 B -3 B
link.html gzip 955 B 953 B -2 B
withRouter.html gzip 941 B 938 B -3 B
Overall change 2.85 kB 2.84 kB -8 B

Diffs

Diff for main-795929d..68.module.js
@@ -1313,6 +1313,24 @@
         return res;
       }
 
+      function loadScript(url) {
+        return new Promise((res, rej) => {
+          var script = document.createElement("script");
+
+          if (true && hasNoModule) {
+            script.type = "module";
+          }
+
+          script.crossOrigin = "anonymous";
+          script.src = url;
+          script.onload = res;
+
+          script.onerror = () => rej(pageLoadError(url));
+
+          document.body.appendChild(script);
+        });
+      }
+
       class PageLoader {
         constructor(buildId, assetPrefix, initialPage, initialStyleSheets) {
           this.initialPage = void 0;
@@ -1377,10 +1395,7 @@
               ? m[route].map(url =>
                   "".concat(this.assetPrefix, "/_next/").concat(encodeURI(url))
                 )
-              : (this.pageRegisterEvents.emit(route, {
-                  error: pageLoadError(route)
-                }),
-                []);
+              : Promise.reject(pageLoadError(route));
           });
         }
         /**
@@ -1514,57 +1529,55 @@
               this.loadingRoutes[route] = true;
 
               if (true) {
-                this.getDependencies(route).then(deps => {
-                  deps.forEach(d => {
-                    if (
-                      d.endsWith(".js") &&
-                      !document.querySelector('script[src^="'.concat(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="'
-                          .concat(relPreload, '"][href^="')
-                          .concat(d, '"]')
-                      )
-                    ) {
-                      appendLink(d, relPreload, "style").catch(() => {
-                        /* ignore preload error */
-                      });
-                    }
+                this.getDependencies(route)
+                  .then(deps => {
+                    var pending = [];
+                    deps.forEach(d => {
+                      if (
+                        d.endsWith(".js") &&
+                        !document.querySelector('script[src^="'.concat(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="'
+                            .concat(relPreload, '"][href^="')
+                            .concat(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 {
                 var url, scriptRoute;
               }
             }
           });
-        }
-
-        loadScript(url, route) {
-          var script = document.createElement("script");
-
-          if (true && hasNoModule) {
-            script.type = "module";
-          }
-
-          script.crossOrigin = "anonymous";
-          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, regFn) {
Diff for main-fbef6ed..30274b71a.js
@@ -1697,6 +1697,26 @@ _N_E = (window["webpackJsonp_N_E"] = window["webpackJsonp_N_E"] || []).push([
         return res;
       }
 
+      function loadScript(url) {
+        return new Promise(function(res, rej) {
+          var script = document.createElement("script");
+
+          if (true && hasNoModule) {
+            script.type = "module";
+          }
+
+          script.crossOrigin = "anonymous";
+          script.src = url;
+          script.onload = res;
+
+          script.onerror = function() {
+            return rej(pageLoadError(url));
+          };
+
+          document.body.appendChild(script);
+        });
+      }
+
       var PageLoader = /*#__PURE__*/ (function() {
         function PageLoader(
           buildId,
@@ -1777,10 +1797,7 @@ _N_E = (window["webpackJsonp_N_E"] = window["webpackJsonp_N_E"] || []).push([
                         .concat(_this.assetPrefix, "/_next/")
                         .concat(encodeURI(url));
                     })
-                  : (_this.pageRegisterEvents.emit(route, {
-                      error: pageLoadError(route)
-                    }),
-                    []);
+                  : Promise.reject(pageLoadError(route));
               });
             }
             /**
@@ -1944,71 +1961,66 @@ _N_E = (window["webpackJsonp_N_E"] = window["webpackJsonp_N_E"] || []).push([
                   _this4.loadingRoutes[route] = true;
 
                   if (true) {
-                    _this4.getDependencies(route).then(function(deps) {
-                      deps.forEach(function(d) {
-                        if (
-                          d.endsWith(".js") &&
-                          !document.querySelector(
-                            'script[src^="'.concat(d, '"]')
-                          )
-                        ) {
-                          _this4.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="'
-                              .concat(relPreload, '"][href^="')
-                              .concat(d, '"]')
-                          )
-                        ) {
-                          appendLink(d, relPreload, "style")["catch"](
-                            function() {
-                              /* ignore preload error */
-                            }
-                          );
-                        }
+                    _this4
+                      .getDependencies(route)
+                      .then(function(deps) {
+                        var pending = [];
+                        deps.forEach(function(d) {
+                          if (
+                            d.endsWith(".js") &&
+                            !document.querySelector(
+                              'script[src^="'.concat(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="'
+                                .concat(relPreload, '"][href^="')
+                                .concat(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"](
+                              function() {
+                                /* ignore preload error */
+                              }
+                            );
+                          }
+                        });
+                        return Promise.all(pending);
+                      })
+                      ["catch"](function(err) {
+                        // Mark the page as failed to load if any of its required scripts
+                        // fail to load:
+                        _this4.pageCache[route] = {
+                          error: err
+                        };
+                        fire({
+                          error: err
+                        });
                       });
-                    });
                   } else {
                     var url, scriptRoute;
                   }
                 }
               });
-            }
-          },
-          {
-            key: "loadScript",
-            value: function loadScript(url, route) {
-              var _this5 = this;
-
-              var script = document.createElement("script");
-
-              if (true && hasNoModule) {
-                script.type = "module";
-              }
-
-              script.crossOrigin = "anonymous";
-              script.src = url;
-
-              script.onerror = function() {
-                _this5.pageRegisterEvents.emit(route, {
-                  error: pageLoadError(url)
-                });
-              };
-
-              document.body.appendChild(script);
             } // This method if called by the route code.
           },
           {
             key: "registerPage",
             value: function registerPage(route, regFn) {
-              var _this6 = this;
+              var _this5 = this;
 
               var register = function register(styleSheets) {
                 try {
@@ -2018,15 +2030,15 @@ _N_E = (window["webpackJsonp_N_E"] = window["webpackJsonp_N_E"] || []).push([
                     mod: mod,
                     styleSheets: styleSheets
                   };
-                  _this6.pageCache[route] = pageData;
+                  _this5.pageCache[route] = pageData;
 
-                  _this6.pageRegisterEvents.emit(route, pageData);
+                  _this5.pageRegisterEvents.emit(route, pageData);
                 } catch (error) {
-                  _this6.pageCache[route] = {
+                  _this5.pageCache[route] = {
                     error: error
                   };
 
-                  _this6.pageRegisterEvents.emit(route, {
+                  _this5.pageRegisterEvents.emit(route, {
                     error: error
                   });
                 }
@@ -2052,11 +2064,11 @@ _N_E = (window["webpackJsonp_N_E"] = window["webpackJsonp_N_E"] || []).push([
                   );
                 },
                 function(error) {
-                  _this6.pageCache[route] = {
+                  _this5.pageCache[route] = {
                     error: error
                   };
 
-                  _this6.pageRegisterEvents.emit(route, {
+                  _this5.pageRegisterEvents.emit(route, {
                     error: error
                   });
                 }
@@ -2070,7 +2082,7 @@ _N_E = (window["webpackJsonp_N_E"] = window["webpackJsonp_N_E"] || []).push([
           {
             key: "prefetch",
             value: function prefetch(route, isDependency) {
-              var _this7 = this;
+              var _this6 = this;
 
               // https://github.com/GoogleChromeLabs/quicklink/blob/453a661fa1fa940e2d2e044452398e38c67a98fb/src/index.mjs#L115-L118
               // License: Apache 2.0
@@ -2112,7 +2124,7 @@ _N_E = (window["webpackJsonp_N_E"] = window["webpackJsonp_N_E"] || []).push([
                         this.getDependencies(route).then(function(urls) {
                           return Promise.all(
                             urls.map(function(dependencyUrl) {
-                              return _this7.prefetch(dependencyUrl, true);
+                              return _this6.prefetch(dependencyUrl, true);
                             })
                           );
                         })
Diff for index.html
@@ -6,7 +6,7 @@
     <meta name="next-head-count" content="2" />
     <link
       rel="preload"
-      href="/_next/static/chunks/main-795929d87ed798e41168.module.js"
+      href="/_next/static/chunks/main-2ff3f81fff451af57216.module.js"
       as="script"
       crossorigin="anonymous"
     />
@@ -81,13 +81,13 @@
       src="/_next/static/chunks/polyfills-f73ba3fc145972ef83e9.js"
     ></script>
     <script
-      src="/_next/static/chunks/main-fbef6ed026230274b71a.js"
+      src="/_next/static/chunks/main-74c305815b3a3f708e65.js"
       async=""
       crossorigin="anonymous"
       nomodule=""
     ></script>
     <script
-      src="/_next/static/chunks/main-795929d87ed798e41168.module.js"
+      src="/_next/static/chunks/main-2ff3f81fff451af57216.module.js"
       async=""
       crossorigin="anonymous"
       type="module"
Diff for link.html
@@ -6,7 +6,7 @@
     <meta name="next-head-count" content="2" />
     <link
       rel="preload"
-      href="/_next/static/chunks/main-795929d87ed798e41168.module.js"
+      href="/_next/static/chunks/main-2ff3f81fff451af57216.module.js"
       as="script"
       crossorigin="anonymous"
     />
@@ -86,13 +86,13 @@
       src="/_next/static/chunks/polyfills-f73ba3fc145972ef83e9.js"
     ></script>
     <script
-      src="/_next/static/chunks/main-fbef6ed026230274b71a.js"
+      src="/_next/static/chunks/main-74c305815b3a3f708e65.js"
       async=""
       crossorigin="anonymous"
       nomodule=""
     ></script>
     <script
-      src="/_next/static/chunks/main-795929d87ed798e41168.module.js"
+      src="/_next/static/chunks/main-2ff3f81fff451af57216.module.js"
       async=""
       crossorigin="anonymous"
       type="module"
Diff for withRouter.html
@@ -6,7 +6,7 @@
     <meta name="next-head-count" content="2" />
     <link
       rel="preload"
-      href="/_next/static/chunks/main-795929d87ed798e41168.module.js"
+      href="/_next/static/chunks/main-2ff3f81fff451af57216.module.js"
       as="script"
       crossorigin="anonymous"
     />
@@ -81,13 +81,13 @@
       src="/_next/static/chunks/polyfills-f73ba3fc145972ef83e9.js"
     ></script>
     <script
-      src="/_next/static/chunks/main-fbef6ed026230274b71a.js"
+      src="/_next/static/chunks/main-74c305815b3a3f708e65.js"
       async=""
       crossorigin="anonymous"
       nomodule=""
     ></script>
     <script
-      src="/_next/static/chunks/main-795929d87ed798e41168.module.js"
+      src="/_next/static/chunks/main-2ff3f81fff451af57216.module.js"
       async=""
       crossorigin="anonymous"
       type="module"

Serverless Mode (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary Timer/next.js refactor/page-loader Change
buildDuration 11.6s 11.6s -24ms
nodeModulesSize 57.7 MB 57.7 MB ⚠️ +1.73 kB
Client Bundles (main, webpack, commons) Overall increase ⚠️
vercel/next.js canary Timer/next.js refactor/page-loader Change
677f882d2ed8..4e55.js gzip 10.2 kB 10.2 kB
framework.HASH.js gzip 39 kB 39 kB
main-ff9b23f..5a93.js gzip 7.17 kB N/A N/A
webpack-e067..f178.js gzip 751 B 751 B
main-6b32d04..be2c.js gzip N/A 7.2 kB N/A
Overall change 57.1 kB 57.2 kB ⚠️ +30 B
Client Bundles (main, webpack, commons) Modern Overall increase ⚠️
vercel/next.js canary Timer/next.js refactor/page-loader Change
677f882d2ed8..dule.js gzip 6.11 kB 6.11 kB
framework.HA..dule.js gzip 39 kB 39 kB
main-b54ffe3..dule.js gzip 6.2 kB N/A N/A
webpack-07c5..dule.js gzip 751 B 751 B
main-397a2a0..dule.js gzip N/A 6.21 kB N/A
Overall change 52 kB 52 kB ⚠️ +10 B
Legacy Client Bundles (polyfills)
vercel/next.js canary Timer/next.js refactor/page-loader Change
polyfills-4b..e242.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary Timer/next.js refactor/page-loader Change
_app-9a0b9e1..b37e.js gzip 1.28 kB 1.28 kB
_error-1464c..a26f.js gzip 3.44 kB 3.44 kB
hooks-89731c..c609.js gzip 887 B 887 B
index-17468f..5d83.js gzip 227 B 227 B
link-000f151..65d4.js gzip 1.29 kB 1.29 kB
routerDirect..924c.js gzip 284 B 284 B
withRouter-7..c13d.js gzip 284 B 284 B
Overall change 7.69 kB 7.69 kB
Client Pages Modern
vercel/next.js canary Timer/next.js refactor/page-loader Change
_app-75d3a82..dule.js gzip 625 B 625 B
_error-e550f..dule.js gzip 2.29 kB 2.29 kB
hooks-cbf13f..dule.js gzip 387 B 387 B
index-b9a643..dule.js gzip 226 B 226 B
link-4cfda7a..dule.js gzip 1.26 kB 1.26 kB
routerDirect..dule.js gzip 284 B 284 B
withRouter-f..dule.js gzip 282 B 282 B
Overall change 5.35 kB 5.35 kB
Client Build Manifests
vercel/next.js canary Timer/next.js refactor/page-loader Change
_buildManifest.js gzip 322 B 322 B
_buildManife..dule.js gzip 329 B 329 B
Overall change 651 B 651 B
Serverless bundles
vercel/next.js canary Timer/next.js refactor/page-loader Change
_error.js 1.03 MB 1.03 MB
404.html 4.18 kB 4.18 kB
hooks.html 3.82 kB 3.82 kB
index.js 1.03 MB 1.03 MB
link.js 1.07 MB 1.07 MB
routerDirect.js 1.07 MB 1.07 MB
withRouter.js 1.07 MB 1.07 MB
Overall change 5.27 MB 5.27 MB
Commit: 4a5e0ee

@kodiakhq kodiakhq bot merged commit 0a72d14 into vercel:canary Aug 19, 2020
@Timer Timer deleted the refactor/page-loader branch August 19, 2020 13:33
m-lautenbach pushed a commit to m-lautenbach/next.js that referenced this pull request Aug 20, 2020
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 vercel#16333
@vercel vercel locked as resolved and limited conversation to collaborators Jan 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Next.js router can sometimes hang for existing users on app after a new deployment
3 participants