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

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

Closed
Timer opened this issue Aug 19, 2020 · 1 comment · Fixed by #16334
Closed

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

Timer opened this issue Aug 19, 2020 · 1 comment · Fixed by #16334
Assignees
Milestone

Comments

@Timer
Copy link
Member

Timer commented Aug 19, 2020

The Next.js router can sometimes hang (an edge case) when a user is on the app and a new deployment is made. This causes routing assets to go missing, and under certain conditions, prevents Next.js from automatically reloading to the new version.

@Timer Timer added the point: 3 label Aug 19, 2020
@Timer Timer added this to the iteration 7 milestone Aug 19, 2020
@kodiakhq kodiakhq bot closed this as completed in #16334 Aug 19, 2020
kodiakhq bot pushed a commit that referenced this issue 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
m-lautenbach pushed a commit to m-lautenbach/next.js that referenced this issue 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
@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants