Fix top-level router update forcing Suspense client fallbacks #33861
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gaearon
requested review from
huozhi,
ijjk,
shuding and
timneutkens
as code owners
This was referenced Feb 1, 2022
Failing test suitesCommit: 05f15cd test/integration/auto-export-serverless/test/index.test.js
Expand output● Auto Export Serverless › Refreshes query on mount test/integration/auto-export/test/index.test.js
Expand output● Auto Export › production › Refreshes query on mount ● Auto Export › production › should update asPath after mount ● Auto Export › dev › Refreshes query on mount ● Auto Export › dev › should update asPath after mount test/e2e/basepath.test.ts
Expand output● basePath › should navigate back correctly to a dynamic route ● basePath › should update dynamic params after mount correctly ● basePath › should navigate to index page with getStaticProps ● basePath › should navigate to nested index page with getStaticProps ● basePath › should work with normal dynamic page ● basePath › should work with catch-all page ● basePath › should fetch data for getStaticProps without reloading ● basePath › should fetch data for getServerSideProps without reloading test/integration/i18n-support-fallback-rewrite-legacy/test/index.test.js
Expand output● i18n Support › dev mode › should not rewrite for index page ● i18n Support › dev mode › should not rewrite for dynamic page ● i18n Support › production mode › should not rewrite for index page ● i18n Support › production mode › should not rewrite for dynamic page |
Stats from current PRDefault Build (Increase detected
|
| vercel/next.js canary | gaearon/next.js patch-1 | Change | |
|---|---|---|---|
| buildDuration | 11.4s | 11.3s | -126ms |
| buildDurationCached | 2.9s | 2.9s | |
| nodeModulesSize | 358 MB | 358 MB |
Page Load Tests Overall decrease ⚠️
| vercel/next.js canary | gaearon/next.js patch-1 | Change | |
|---|---|---|---|
| / failed reqs | 0 | 0 | ✓ |
| / total time (seconds) | 2.641 | 2.728 | |
| / avg req/sec | 946.78 | 916.36 | |
| /error-in-render failed reqs | 0 | 0 | ✓ |
| /error-in-render total time (seconds) | 1.261 | 1.268 | |
| /error-in-render avg req/sec | 1981.97 | 1971.76 |
Client Bundles (main, webpack, commons) Overall increase ⚠️
| vercel/next.js canary | gaearon/next.js patch-1 | Change | |
|---|---|---|---|
| 450.HASH.js gzip | 179 B | 179 B | ✓ |
| framework-HASH.js gzip | 42.2 kB | 42.2 kB | ✓ |
| main-HASH.js gzip | 27.2 kB | 27.2 kB | |
| webpack-HASH.js gzip | 1.44 kB | 1.44 kB | ✓ |
| Overall change | 71 kB | 71 kB |
Legacy Client Bundles (polyfills)
| vercel/next.js canary | gaearon/next.js patch-1 | Change | |
|---|---|---|---|
| polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
| Overall change | 31 kB | 31 kB | ✓ |
Client Pages
| vercel/next.js canary | gaearon/next.js patch-1 | Change | |
|---|---|---|---|
| _app-HASH.js gzip | 1.37 kB | 1.37 kB | ✓ |
| _error-HASH.js gzip | 194 B | 194 B | ✓ |
| amp-HASH.js gzip | 312 B | 312 B | ✓ |
| css-HASH.js gzip | 326 B | 326 B | ✓ |
| dynamic-HASH.js gzip | 2.37 kB | 2.37 kB | ✓ |
| head-HASH.js gzip | 350 B | 350 B | ✓ |
| hooks-HASH.js gzip | 919 B | 919 B | ✓ |
| image-HASH.js gzip | 4.94 kB | 4.94 kB | ✓ |
| index-HASH.js gzip | 263 B | 263 B | ✓ |
| link-HASH.js gzip | 2.19 kB | 2.19 kB | ✓ |
| routerDirect..HASH.js gzip | 321 B | 321 B | ✓ |
| script-HASH.js gzip | 383 B | 383 B | ✓ |
| withRouter-HASH.js gzip | 318 B | 318 B | ✓ |
| 85e02e95b279..7e3.css gzip | 107 B | 107 B | ✓ |
| Overall change | 14.4 kB | 14.4 kB | ✓ |
Client Build Manifests
| vercel/next.js canary | gaearon/next.js patch-1 | Change | |
|---|---|---|---|
| _buildManifest.js gzip | 459 B | 459 B | ✓ |
| Overall change | 459 B | 459 B | ✓ |
Rendered Page Sizes Overall increase ⚠️
| vercel/next.js canary | gaearon/next.js patch-1 | Change | |
|---|---|---|---|
| index.html gzip | 530 B | 530 B | ✓ |
| link.html gzip | 543 B | 545 B | |
| withRouter.html gzip | 525 B | 526 B | |
| Overall change | 1.6 kB | 1.6 kB |
Diffs
Diff for main-HASH.js
@@ -700,6 +700,7 @@
var webpackHMR;
var router;
exports.router = router;
+ var publicRouterInstance;
var CachedApp, onPerfEntry;
headManager.getIsSsr = function() {
return router.isSsr;
@@ -973,6 +974,8 @@
isPreview: isPreview
}
);
+ publicRouterInstance = (0,
+ _router1).makePublicRouterInstance(router);
renderCtx = {
App: CachedApp,
initial: true,
@@ -984,12 +987,12 @@
}
render(renderCtx);
return _ctx.abrupt("return", emitter);
- case 44:
+ case 45:
return _ctx.abrupt("return", {
emitter: emitter,
renderCtx: renderCtx
});
- case 45:
+ case 46:
case "end":
return _ctx.stop();
}
@@ -1226,7 +1229,7 @@
/*#__PURE__*/ _react.default.createElement(
_routerContext.RouterContext.Provider,
{
- value: (0, _router1).makePublicRouterInstance(router)
+ value: publicRouterInstance
},
/*#__PURE__*/ _react.default.createElement(
_headManagerContext.HeadManagerContext.Provider,Diff for index.html
@@ -19,7 +19,7 @@
defer=""
></script>
<script
- src="/_next/static/chunks/main-c6fc133fe313f8ea.js"
+ src="/_next/static/chunks/main-5f632ae85152c91f.js"
defer=""
></script>
<scriptDiff for link.html
@@ -19,7 +19,7 @@
defer=""
></script>
<script
- src="/_next/static/chunks/main-c6fc133fe313f8ea.js"
+ src="/_next/static/chunks/main-5f632ae85152c91f.js"
defer=""
></script>
<scriptDiff for withRouter.html
@@ -19,7 +19,7 @@
defer=""
></script>
<script
- src="/_next/static/chunks/main-c6fc133fe313f8ea.js"
+ src="/_next/static/chunks/main-5f632ae85152c91f.js"
defer=""
></script>
<scriptDefault Build with SWC (Decrease detected ✓)
General Overall increase ⚠️
| vercel/next.js canary | gaearon/next.js patch-1 | Change | |
|---|---|---|---|
| buildDuration | 14.5s | 14.2s | -266ms |
| buildDurationCached | 2.9s | 2.9s | -43ms |
| nodeModulesSize | 358 MB | 358 MB |
Page Load Tests Overall decrease ⚠️
| vercel/next.js canary | gaearon/next.js patch-1 | Change | |
|---|---|---|---|
| / failed reqs | 0 | 0 | ✓ |
| / total time (seconds) | 2.677 | 2.73 | |
| / avg req/sec | 934.05 | 915.6 | |
| /error-in-render failed reqs | 0 | 0 | ✓ |
| /error-in-render total time (seconds) | 1.253 | 1.255 | 0 |
| /error-in-render avg req/sec | 1995.07 | 1992.7 |
Client Bundles (main, webpack, commons) Overall increase ⚠️
| vercel/next.js canary | gaearon/next.js patch-1 | Change | |
|---|---|---|---|
| 450.HASH.js gzip | 179 B | 179 B | ✓ |
| framework-HASH.js gzip | 42.3 kB | 42.3 kB | ✓ |
| main-HASH.js gzip | 27.3 kB | 27.3 kB | |
| webpack-HASH.js gzip | 1.44 kB | 1.44 kB | ✓ |
| Overall change | 71.3 kB | 71.3 kB |
Legacy Client Bundles (polyfills)
| vercel/next.js canary | gaearon/next.js patch-1 | Change | |
|---|---|---|---|
| polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
| Overall change | 31 kB | 31 kB | ✓ |
Client Pages
| vercel/next.js canary | gaearon/next.js patch-1 | Change | |
|---|---|---|---|
| _app-HASH.js gzip | 1.35 kB | 1.35 kB | ✓ |
| _error-HASH.js gzip | 180 B | 180 B | ✓ |
| amp-HASH.js gzip | 305 B | 305 B | ✓ |
| css-HASH.js gzip | 321 B | 321 B | ✓ |
| dynamic-HASH.js gzip | 2.36 kB | 2.36 kB | ✓ |
| head-HASH.js gzip | 342 B | 342 B | ✓ |
| hooks-HASH.js gzip | 911 B | 911 B | ✓ |
| image-HASH.js gzip | 4.98 kB | 4.98 kB | ✓ |
| index-HASH.js gzip | 256 B | 256 B | ✓ |
| link-HASH.js gzip | 2.21 kB | 2.21 kB | ✓ |
| routerDirect..HASH.js gzip | 314 B | 314 B | ✓ |
| script-HASH.js gzip | 375 B | 375 B | ✓ |
| withRouter-HASH.js gzip | 309 B | 309 B | ✓ |
| 85e02e95b279..7e3.css gzip | 107 B | 107 B | ✓ |
| Overall change | 14.3 kB | 14.3 kB | ✓ |
Client Build Manifests
| vercel/next.js canary | gaearon/next.js patch-1 | Change | |
|---|---|---|---|
| _buildManifest.js gzip | 458 B | 458 B | ✓ |
| Overall change | 458 B | 458 B | ✓ |
Rendered Page Sizes Overall increase ⚠️
| vercel/next.js canary | gaearon/next.js patch-1 | Change | |
|---|---|---|---|
| index.html gzip | 530 B | 531 B | |
| link.html gzip | 543 B | 544 B | |
| withRouter.html gzip | 525 B | 525 B | ✓ |
| Overall change | 1.6 kB | 1.6 kB |
Diffs
Diff for main-HASH.js
@@ -700,6 +700,7 @@
var webpackHMR;
var router;
exports.router = router;
+ var publicRouterInstance;
var CachedApp, onPerfEntry;
headManager.getIsSsr = function() {
return router.isSsr;
@@ -973,6 +974,8 @@
isPreview: isPreview
}
);
+ publicRouterInstance = (0,
+ _router1).makePublicRouterInstance(router);
renderCtx = {
App: CachedApp,
initial: true,
@@ -984,12 +987,12 @@
}
render(renderCtx);
return _ctx.abrupt("return", emitter);
- case 44:
+ case 45:
return _ctx.abrupt("return", {
emitter: emitter,
renderCtx: renderCtx
});
- case 45:
+ case 46:
case "end":
return _ctx.stop();
}
@@ -1226,7 +1229,7 @@
/*#__PURE__*/ _react.default.createElement(
_routerContext.RouterContext.Provider,
{
- value: (0, _router1).makePublicRouterInstance(router)
+ value: publicRouterInstance
},
/*#__PURE__*/ _react.default.createElement(
_headManagerContext.HeadManagerContext.Provider,Diff for index.html
@@ -19,7 +19,7 @@
defer=""
></script>
<script
- src="/_next/static/chunks/main-c6fc133fe313f8ea.js"
+ src="/_next/static/chunks/main-5f632ae85152c91f.js"
defer=""
></script>
<scriptDiff for link.html
@@ -19,7 +19,7 @@
defer=""
></script>
<script
- src="/_next/static/chunks/main-c6fc133fe313f8ea.js"
+ src="/_next/static/chunks/main-5f632ae85152c91f.js"
defer=""
></script>
<scriptDiff for withRouter.html
@@ -19,7 +19,7 @@
defer=""
></script>
<script
- src="/_next/static/chunks/main-c6fc133fe313f8ea.js"
+ src="/_next/static/chunks/main-5f632ae85152c91f.js"
defer=""
></script>
<script
|
looks like this fails a bunch of tests? i'm not sure what to make of these failures or how to run the same tests locally.. |
ijjk
reviewed
Feb 2, 2022
| @@ -413,6 +415,7 @@ export async function initNext(opts: { webpackHMR?: any } = {}) { | |||
| domainLocales, | |||
| isPreview, | |||
| }) | |||
| publicRouterInstance = makePublicRouterInstance(router) | |||
There was a problem hiding this comment.
We might need to tweak how makePublicRouterInstance is handling the fields to prevent this from being re-created on every render. Currently it's creating a new instance and copying the fields to prevent them from being treated as stateful.
This also causes the router methods to change on each render, related issue here #18127
There was a problem hiding this comment.
Doesn't putting it outside of render and creating it once at initialization, like this PR does, mitigate the issue?
There was a problem hiding this comment.
It does but then the fields are never updated from the initial public router since they are copied here
next.js/packages/next/client/router.ts
Lines 160 to 170 in 39d3210
There was a problem hiding this comment.
So, the issue is that router is a bit of a misnomer here. When used globally, it means "routing functions + a reference to the currently visible route." When used locally (via a React hook), it means "routing functions + the currently rendered route."
The router context should change for each tree IMO. Consider something like this:
function Foo() {
const { query } = useRouter()
return <span>{JSON.stringify(query)}</span>
}- If the context didn't change when
querychanged, how wouldFoorerender? - If the value of
queryonly pointed to the active route, how would we supportstartTransition, which renders the next page concurrently in the background, and presumably wants the newqueryvalue?
My approach in #33875 is working on addressing this from this perspective, i.e. just creating a new public instance for each route/navigation, instead of generating a new one each time AppContainer is rerendered.
The approach in this PR is probably fine to unblock for now, but I don't think it'll work for concurrent rendering.
Were there any specific issues you ran into while trying to run the tests locally? We document some ways to run them in the contributing doc and if there's any tips we can add there let us know. You can also run |
|
@gaearon can you confirm whether this is still occurring with the latest canary? If so, could you provide a full repro case? |
|
I'm gonna close this per above, please let us know if this is still an issue and we can investigate further! |
|
Was there some change that would cause it to not occur? My understanding is that the problem is clear-cut: always recreating the instance in that component causes unnecessary context updates. Unnecessary context updates at the top level are bad. Which part needs a reproduction? |
|
OK, so I can consistently reproduce it locally with a minimal repro on this branch: https://github.com/gaearon/sc-bug-repro/pull/new/router-context-bug (disregard the repo name; it's not SC-specific) However, that repro only works with 12.1.0. When I upgrade to 12.1.7-canary.11, it doesn't repro. I'm not sure if the problem is gone per se or if it's just that it's harder to repro now. |
|
The reason I was previously getting an extra route update at startup was this code: next.js/packages/next/client/index.tsx Lines 100 to 121 in d25e246 (I had some "rewrites" in the config.) There's still a top-level context update now (which seems unnecessary? nothing changed in my app's state). But at least this new next.js/packages/next/client/index.tsx Lines 553 to 556 in d25e246 (and this also fixes the dehydration issue) However, ideally, there wouldn't be another top-level update at all if nothing changed. |
|
I'm going to disable the |
1 task
|
Filed #37139 for the general issue. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This top-level component recreates the public router instance when it renders. I'm hoping this is unnecessary (but I might be wrong). I'm observing this because I'm seeing dehydrated Suspense HTML content being replaced with client fallbacks. This is known (and expected) to happen when there are top-level context updates before hydration is finished facebook/react#22692. So we should avoid top-level context updates unless something meaningful actually changed.
How to test
I typed this on GH and didn't actually "test" this change.
However, I tested a similar change locally, and it solved the issue with a minimal repro. It did not solve the issue in my real project which I suspect means there's more top-level context updates somewhere (either in Next.js or in my app). But this is a start.
Locally, my test was basically returning
I am running an
experimentalbuild of React. I verified that content from<Something />appeared in HTML but was getting deleted soon after hydration (but not in the hydration commit). There were no mismatches. After fixing this context provider, Suspense was no longer forced into fallbacks in my minimal repro. This would be a good regression test to add to Next.js.