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

Prevent timeout when loading routes in development #25749

Merged
merged 4 commits into from
Jul 12, 2021

Conversation

stovmascript
Copy link
Contributor

@stovmascript stovmascript commented Jun 3, 2021

@Timer The new route loader (#19006) will timeout loading routes after 3.8s MS_MAX_IDLE_DELAY, but this can easily happen when running dev on a large app.

Fixes #25675

Bug

  • Related issues linked using fixes #number
  • Integration tests added

Feature

  • Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • Related issues linked using fixes #number
  • Integration tests added
  • Documentation added
  • Telemetry added. In case of a feature if it's used or not.

Documentation / Examples

  • Make sure the linting passes

@ijjk

This comment has been minimized.

@tonytoth
Copy link

tonytoth commented Jul 5, 2021

I agree with @stovmascript but I am not sure why (for me) it does take > 3.8 only in docker. On my local machine it works nice.

The new route loader (vercel#19006) will timeout
loading routes after 3.8s (MS_MAX_IDLE_DELAY), but this can easily happen when
running dev on a large app.

Fixes vercel#25675
@ijjk

This comment has been minimized.

Copy link
Contributor Author

@stovmascript stovmascript left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these changes look good then? Happy to improve something if need be :)

Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we can tweak this a bit more to prevent adding any changes to the production code, I attached a diff here as I'm not able to update this PR directly since contributions from maintainers is disabled for this PR.

diff --git a/packages/next/client/route-loader.ts b/packages/next/client/route-loader.ts
index 14cb8bef3..7d09f0414 100644
--- a/packages/next/client/route-loader.ts
+++ b/packages/next/client/route-loader.ts
@@ -149,6 +149,38 @@ function appendScript(
   })
 }
 
+// we wait for pages to be built in dev before we start
+// the route transition timeout to prevent an un-necessary
+// hard navigation in development
+let devBuildPromise: Promise<void> | undefined
+let devBuildResolve: (() => void) | undefined
+
+if (process.env.NODE_ENV === 'development') {
+  const { addMessageListener } = require('./dev/error-overlay/eventsource')
+
+  addMessageListener((event: any) => {
+    // This is the heartbeat event
+    if (event.data === '\uD83D\uDC93') {
+      return
+    }
+
+    const obj =
+      typeof event === 'string' ? { action: event } : JSON.parse(event.data)
+
+    switch (obj.action) {
+      case 'built':
+      case 'sync':
+        if (devBuildResolve) {
+          devBuildResolve()
+          devBuildResolve = undefined
+        }
+        break
+      default:
+        break
+    }
+  })
+}
+
 // Resolve a promise that times out after given amount of milliseconds.
 function resolvePromiseWithTimeout<T>(
   p: Promise<T>,
@@ -164,13 +196,29 @@ function resolvePromiseWithTimeout<T>(
       resolve(r)
     }).catch(reject)
 
-    requestIdleCallback(() =>
-      setTimeout(() => {
-        if (!cancelled) {
-          reject(err)
-        }
-      }, ms)
-    )
+    // we wrap these checks separately for better dead-code elimination
+    // in production bundles
+    if (process.env.NODE_ENV === 'development') {
+      ;(devBuildPromise || Promise.resolve()).then(() => {
+        requestIdleCallback(() =>
+          setTimeout(() => {
+            if (!cancelled) {
+              reject(err)
+            }
+          }, ms)
+        )
+      })
+    }
+
+    if (process.env.NODE_ENV !== 'development') {
+      requestIdleCallback(() =>
+        setTimeout(() => {
+          if (!cancelled) {
+            reject(err)
+          }
+        }, ms)
+      )
+    }
   })
 }
 
@@ -307,6 +355,12 @@ function createRouteLoader(assetPrefix: string): RouteLoader {
     },
     loadRoute(route: string, prefetch?: boolean) {
       return withFuture<RouteLoaderEntry>(route, routes, () => {
+        if (process.env.NODE_ENV === 'development') {
+          devBuildPromise = new Promise<void>((resolve) => {
+            devBuildResolve = resolve
+          })
+        }
+
         return resolvePromiseWithTimeout(
           getFilesForRoute(assetPrefix, route)
             .then(({ scripts, css }) => {

@stovmascript
Copy link
Contributor Author

@ijjk Done

@ijjk
Copy link
Member

ijjk commented Jul 12, 2021

Stats from current PR

Default Build (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary monitora-media/next.js fix-route-loader-dev Change
buildDuration 13.7s 13.9s ⚠️ +193ms
buildDurationCached 3.5s 3.4s -132ms
nodeModulesSize 49.3 MB 49.3 MB ⚠️ +3.75 kB
Page Load Tests Overall decrease ⚠️
vercel/next.js canary monitora-media/next.js fix-route-loader-dev Change
/ failed reqs 0 0
/ total time (seconds) 2.403 2.428 ⚠️ +0.02
/ avg req/sec 1040.4 1029.76 ⚠️ -10.64
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.369 1.387 ⚠️ +0.02
/error-in-render avg req/sec 1826.58 1801.87 ⚠️ -24.71
Client Bundles (main, webpack, commons)
vercel/next.js canary monitora-media/next.js fix-route-loader-dev Change
359.HASH.js gzip 3.09 kB 3.09 kB
framework-HASH.js gzip 42 kB 42 kB
main-HASH.js gzip 20.6 kB 20.6 kB
webpack-HASH.js gzip 1.49 kB 1.49 kB
Overall change 67.2 kB 67.2 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary monitora-media/next.js fix-route-loader-dev Change
polyfills-HASH.js gzip 31.1 kB 31.1 kB
Overall change 31.1 kB 31.1 kB
Client Pages
vercel/next.js canary monitora-media/next.js fix-route-loader-dev Change
_app-HASH.js gzip 803 B 803 B
_error-HASH.js gzip 3.18 kB 3.18 kB
amp-HASH.js gzip 526 B 526 B
css-HASH.js gzip 329 B 329 B
hooks-HASH.js gzip 903 B 903 B
index-HASH.js gzip 261 B 261 B
link-HASH.js gzip 1.66 kB 1.66 kB
routerDirect..HASH.js gzip 319 B 319 B
withRouter-HASH.js gzip 320 B 320 B
bb14e60e810b..30f.css gzip 125 B 125 B
Overall change 8.42 kB 8.42 kB
Client Build Manifests
vercel/next.js canary monitora-media/next.js fix-route-loader-dev Change
_buildManifest.js gzip 390 B 390 B
Overall change 390 B 390 B
Rendered Page Sizes
vercel/next.js canary monitora-media/next.js fix-route-loader-dev Change
index.html gzip 531 B 531 B
link.html gzip 544 B 544 B
withRouter.html gzip 525 B 525 B
Overall change 1.6 kB 1.6 kB

Diffs

Diff for main-HASH.js
@@ -2459,6 +2459,14 @@
           script.src = src;
           document.body.appendChild(script);
         });
+      } // We wait for pages to be built in dev before we start the route transition
+      // timeout to prevent an un-necessary hard navigation in development.
+
+      var devBuildPromise;
+      var devBuildResolve;
+
+      if (false) {
+        var _require, addMessageListener;
       } // Resolve a promise that times out after given amount of milliseconds.
 
       function resolvePromiseWithTimeout(p, ms, err) {
@@ -2468,14 +2476,21 @@
             // Resolved, cancel the timeout
             cancelled = true;
             resolve(r);
-          })["catch"](reject);
-          (0, _requestIdleCallback.requestIdleCallback)(function() {
-            return setTimeout(function() {
-              if (!cancelled) {
-                reject(err);
-              }
-            }, ms);
-          });
+          })["catch"](reject); // We wrap these checks separately for better dead-code elimination in
+          // production bundles.
+
+          if (false) {
+          }
+
+          if (true) {
+            (0, _requestIdleCallback.requestIdleCallback)(function() {
+              return setTimeout(function() {
+                if (!cancelled) {
+                  reject(err);
+                }
+              }, ms);
+            });
+          }
         });
       } // TODO: stop exporting or cache the failure
       // It'd be best to stop exporting this. It's an implementation detail. We're
@@ -2612,6 +2627,9 @@
             var _this = this;
 
             return withFuture(route, routes, function() {
+              if (false) {
+              }
+
               return resolvePromiseWithTimeout(
                 getFilesForRoute(assetPrefix, route)
                   .then(function(_ref) {
Diff for index.html
@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-4f15dc261f5ce5c05417.js"
+      src="/_next/static/chunks/main-ae652afd74f33a5448c1.js"
       defer=""
     ></script>
     <script
Diff for link.html
@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-4f15dc261f5ce5c05417.js"
+      src="/_next/static/chunks/main-ae652afd74f33a5448c1.js"
       defer=""
     ></script>
     <script
Diff for withRouter.html
@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-4f15dc261f5ce5c05417.js"
+      src="/_next/static/chunks/main-ae652afd74f33a5448c1.js"
       defer=""
     ></script>
     <script

Webpack 4 Mode (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary monitora-media/next.js fix-route-loader-dev Change
buildDuration 11.8s 12.4s ⚠️ +615ms
buildDurationCached 4.9s 4.6s -304ms
nodeModulesSize 49.3 MB 49.3 MB ⚠️ +3.75 kB
Page Load Tests Overall decrease ⚠️
vercel/next.js canary monitora-media/next.js fix-route-loader-dev Change
/ failed reqs 0 0
/ total time (seconds) 2.425 2.436 ⚠️ +0.01
/ avg req/sec 1030.9 1026.35 ⚠️ -4.55
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.349 1.388 ⚠️ +0.04
/error-in-render avg req/sec 1852.71 1801.18 ⚠️ -51.53
Client Bundles (main, webpack, commons)
vercel/next.js canary monitora-media/next.js fix-route-loader-dev Change
14.HASH.js gzip 3.11 kB 3.11 kB
677f882d2ed8..HASH.js gzip 13.9 kB 13.9 kB
framework.HASH.js gzip 41.8 kB 41.8 kB
main-HASH.js gzip 7.81 kB 7.81 kB
webpack-HASH.js gzip 1.19 kB 1.19 kB
Overall change 67.8 kB 67.8 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary monitora-media/next.js fix-route-loader-dev Change
polyfills-HASH.js gzip 31.3 kB 31.3 kB
Overall change 31.3 kB 31.3 kB
Client Pages
vercel/next.js canary monitora-media/next.js fix-route-loader-dev Change
_app-HASH.js gzip 791 B 791 B
_error-HASH.js gzip 3.83 kB 3.83 kB
amp-HASH.js gzip 531 B 531 B
css-HASH.js gzip 333 B 333 B
hooks-HASH.js gzip 910 B 910 B
index-HASH.js gzip 230 B 230 B
link-HASH.js gzip 1.64 kB 1.64 kB
routerDirect..HASH.js gzip 297 B 297 B
withRouter-HASH.js gzip 293 B 293 B
e025d2764813..52f.css gzip 125 B 125 B
Overall change 8.98 kB 8.98 kB
Client Build Manifests
vercel/next.js canary monitora-media/next.js fix-route-loader-dev Change
_buildManifest.js gzip 418 B 418 B
Overall change 418 B 418 B
Rendered Page Sizes Overall decrease ✓
vercel/next.js canary monitora-media/next.js fix-route-loader-dev Change
index.html gzip 576 B 575 B -1 B
link.html gzip 587 B 586 B -1 B
withRouter.html gzip 570 B 569 B -1 B
Overall change 1.73 kB 1.73 kB -3 B

Diffs

Diff for 677f882d2ed8..c4df.HASH.js
@@ -980,6 +980,14 @@ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLI
           script.src = src;
           document.body.appendChild(script);
         });
+      } // We wait for pages to be built in dev before we start the route transition
+      // timeout to prevent an un-necessary hard navigation in development.
+
+      var devBuildPromise;
+      var devBuildResolve;
+
+      if (false) {
+        var _require, addMessageListener;
       } // Resolve a promise that times out after given amount of milliseconds.
 
       function resolvePromiseWithTimeout(p, ms, err) {
@@ -989,14 +997,21 @@ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLI
             // Resolved, cancel the timeout
             cancelled = true;
             resolve(r);
-          })["catch"](reject);
-          (0, _requestIdleCallback.requestIdleCallback)(function() {
-            return setTimeout(function() {
-              if (!cancelled) {
-                reject(err);
-              }
-            }, ms);
-          });
+          })["catch"](reject); // We wrap these checks separately for better dead-code elimination in
+          // production bundles.
+
+          if (false) {
+          }
+
+          if (true) {
+            (0, _requestIdleCallback.requestIdleCallback)(function() {
+              return setTimeout(function() {
+                if (!cancelled) {
+                  reject(err);
+                }
+              }, ms);
+            });
+          }
         });
       } // TODO: stop exporting or cache the failure
       // It'd be best to stop exporting this. It's an implementation detail. We're
@@ -1133,6 +1148,9 @@ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLI
             var _this = this;
 
             return withFuture(route, routes, function() {
+              if (false) {
+              }
+
               return resolvePromiseWithTimeout(
                 getFilesForRoute(assetPrefix, route)
                   .then(function(_ref) {
Diff for index.html
@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/677f882d2ed86fa3467b8979053c1a4c3f8bc4df.743faf44f9b7c645e1e6.js"
+      src="/_next/static/chunks/677f882d2ed86fa3467b8979053c1a4c3f8bc4df.7ea47bba50cd89db0708.js"
       defer=""
     ></script>
     <script
Diff for link.html
@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/677f882d2ed86fa3467b8979053c1a4c3f8bc4df.743faf44f9b7c645e1e6.js"
+      src="/_next/static/chunks/677f882d2ed86fa3467b8979053c1a4c3f8bc4df.7ea47bba50cd89db0708.js"
       defer=""
     ></script>
     <script
Diff for withRouter.html
@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/677f882d2ed86fa3467b8979053c1a4c3f8bc4df.743faf44f9b7c645e1e6.js"
+      src="/_next/static/chunks/677f882d2ed86fa3467b8979053c1a4c3f8bc4df.7ea47bba50cd89db0708.js"
       defer=""
     ></script>
     <script
Commit: 2af1953

Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

@ijjk ijjk merged commit 849a79f into vercel:canary Jul 12, 2021
@tonytoth
Copy link

I am not sure that this solution covers all cases. I still have few pages that are reload.

@stovmascript
Copy link
Contributor Author

Same here. There will have to be another source of the routeChangeError though.

@ijjk
Copy link
Member

ijjk commented Jul 15, 2021

Can you provide more information on the case you're seeing reloading in development after upgrading past v11.0.2-canary.12 of Next.js?

A screen recording of the console during the reload or a repo with steps to reproduce consistently can help.

@tonytoth
Copy link

This is the most I can get at the moment (I will try to create a reproduction repo but a bit later) https://streamable.com/1uq88y

jarvelov pushed a commit to jarvelov/next.js that referenced this pull request Jul 22, 2021
The fix in PR vercel#25749 only works some of the times. The reason why it
doesn't work all of the times is because the `devBuildResolve` function
is called when the assets have been built, but this can happen before
the `getFilesForRoute` promise chain has completed, and thus the
`cancelled` variable could not yet have been updated.

To fix this we wait for the `getFilesForRoute` promise chain to be
fulfilled and then resolve the `devBuildPromise` promise afterwards.
This allows the `getFilesForRoute` promise chain to be fulfilled before
and the `cancelled` variable can be updated accordingly.

If an error is thrown somewhere in the `getFilesForRoute` promise chain,
i.e. due to a failed fetch request, then that will also resolve the
`devBuildPromise` and the error will bubble up to ultimately become a
`routeChangeError` which will reload the page.

With this fix the need to listen for HMR events has become obsolete as
regardless of when the HMR build/sync events complete we still want to
ensure that the `getFilesForRoute` promise chain has been fulfilled
before resolving the `devBuildPromise`.
@jarvelov
Copy link
Contributor

jarvelov commented Jul 22, 2021

Just wanted to chime in to say that we're experiencing the same thing with v11.0.2-canary.19. Even with the fix in this PR we're still seeing routeChangeError and the page gets reloaded.

I have tracked down why the routeChangeError still pops up and written a patch which fixes the page reloads. See PR #27395 for a detailed explanation of why some routeChangeErrors are still present in the PR.

jarvelov pushed a commit to jarvelov/next.js that referenced this pull request Jul 22, 2021
The fix in PR vercel#25749 only works some of the time. The reason why it
doesn't work all of the time is because the `devBuildResolve` function
is called when the assets have been built, but this can happen before
the `getFilesForRoute` promise chain has completed, and thus the
`cancelled` variable could not yet have been updated.

To fix this we wait for the `getFilesForRoute` promise chain to be
fulfilled and then resolve the `devBuildPromise` promise afterwards.
This allows the `getFilesForRoute` promise chain to be fulfilled before
and the `cancelled` variable can be updated accordingly.

If an error is thrown somewhere in the `getFilesForRoute` promise chain,
i.e. due to a failed fetch request, then that will also resolve the
`devBuildPromise` and the error will bubble up to ultimately become a
`routeChangeError` which will reload the page.

With this fix the need to listen for HMR events has become obsolete as
regardless of when the HMR build/sync events complete we still want to
ensure that the `getFilesForRoute` promise chain has been fulfilled
before resolving the `devBuildPromise`.
ijjk added a commit that referenced this pull request Jul 23, 2021
…de (#27395)

The fix in PR #25749 only works some of the time. The reason why it
doesn't work all of the time is because the `devBuildResolve` function
is called when the assets have been built, but this can happen before
the `getFilesForRoute` promise chain has completed, and thus the
`cancelled` variable could not yet have been updated.

To fix this we wait for the `getFilesForRoute` promise chain to be
fulfilled and then resolve the `devBuildPromise` promise afterwards.
This allows the `getFilesForRoute` promise chain to be fulfilled before
and the `cancelled` variable can be updated accordingly.

If an error is thrown somewhere in the `getFilesForRoute` promise chain,
i.e. due to a failed fetch request, then that will also resolve the
`devBuildPromise` and the error will bubble up to ultimately become a
`routeChangeError` which will reload the page.

With this fix the need to listen for HMR events has become obsolete as
regardless of when the HMR build/sync events complete we still want to
ensure that the `getFilesForRoute` promise chain has been fulfilled
before resolving the `devBuildPromise`.

Co-authored-by: Tobias Järvelöv <[email protected]>
Co-authored-by: JJ Kasper <[email protected]>
flybayer pushed a commit to blitz-js/next.js that referenced this pull request Aug 19, 2021
* Prevent timeout when loading routes in development

The new route loader (vercel#19006) will timeout
loading routes after 3.8s (MS_MAX_IDLE_DELAY), but this can easily happen when
running dev on a large app.

Fixes vercel#25675

* Delay route loading timeout in development

* refactor: Integrate onBuildCallback into resolvePromiseWithTimeout

* refactor: Tweak for better dead-code elimination

Co-authored-by: JJ Kasper <[email protected]>
flybayer pushed a commit to blitz-js/next.js that referenced this pull request Aug 19, 2021
…de (vercel#27395)

The fix in PR vercel#25749 only works some of the time. The reason why it
doesn't work all of the time is because the `devBuildResolve` function
is called when the assets have been built, but this can happen before
the `getFilesForRoute` promise chain has completed, and thus the
`cancelled` variable could not yet have been updated.

To fix this we wait for the `getFilesForRoute` promise chain to be
fulfilled and then resolve the `devBuildPromise` promise afterwards.
This allows the `getFilesForRoute` promise chain to be fulfilled before
and the `cancelled` variable can be updated accordingly.

If an error is thrown somewhere in the `getFilesForRoute` promise chain,
i.e. due to a failed fetch request, then that will also resolve the
`devBuildPromise` and the error will bubble up to ultimately become a
`routeChangeError` which will reload the page.

With this fix the need to listen for HMR events has become obsolete as
regardless of when the HMR build/sync events complete we still want to
ensure that the `getFilesForRoute` promise chain has been fulfilled
before resolving the `devBuildPromise`.

Co-authored-by: Tobias Järvelöv <[email protected]>
Co-authored-by: JJ Kasper <[email protected]>
@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
@stovmascript stovmascript deleted the fix-route-loader-dev branch March 8, 2022 10:30
@stovmascript stovmascript restored the fix-route-loader-dev branch March 8, 2022 10:33
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.

Error: Route did not complete loading: /some-page
4 participants