Skip to content

Fix top-level router update forcing Suspense client fallbacks #33861

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Fix top-level router update forcing Suspense client fallbacks #33861

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de11318
Fix unnecessary top-level context update in router
gaearon Feb 1, 2022
05f15cd
Maybe fix TS
gaearon Feb 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/next/client/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { HeadManagerContext } from '../shared/lib/head-manager-context'
import mitt, { MittEmitter } from '../shared/lib/mitt'
import { RouterContext } from '../shared/lib/router-context'
import type Router from '../shared/lib/router/router'
import type { NextRouter } from '../shared/lib/router/router'
import {
AppComponent,
AppProps,
Expand Down Expand Up @@ -174,6 +175,7 @@ const appElement: HTMLElement | null = document.getElementById('__next')
let lastRenderReject: (() => void) | null
let webpackHMR: any
export let router: Router
let publicRouterInstance: NextRouter
let CachedApp: AppComponent, onPerfEntry: (metric: any) => void
headManager.getIsSsr = () => {
return router.isSsr
Expand Down Expand Up @@ -413,6 +415,7 @@ export async function initNext(opts: { webpackHMR?: any } = {}) {
domainLocales,
isPreview,
})
publicRouterInstance = makePublicRouterInstance(router)
Copy link
Member

@ijjk ijjk Feb 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't putting it outside of render and creating it once at initialization, like this PR does, mitigate the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, looks like we should go with #33875 approach instead? @devknoll

Copy link
Member

Choose a reason for hiding this comment

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

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

for (const property of urlPropertyFields) {
if (typeof scopedRouter[property] === 'object') {
instance[property] = Object.assign(
Array.isArray(scopedRouter[property]) ? [] : {},
scopedRouter[property]
) // makes sure query is not stateful
continue
}
instance[property] = scopedRouter[property]
}
and then the underlying router instance is updated but not the public router instance. So maybe instead of copying the fields we could Proxy them in development, erroring when setting to them to prevent them from being treated as stateful and in production we don't modify the router like this 🤔

Copy link
Contributor

@devknoll devknoll Feb 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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>
}
  1. If the context didn't change when query changed, how would Foo rerender?
  2. If the value of query only pointed to the active route, how would we support startTransition, which renders the next page concurrently in the background, and presumably wants the new query value?

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.


const renderCtx: RenderRouteInfo = {
App: CachedApp,
Expand Down Expand Up @@ -620,7 +623,7 @@ function AppContainer({
)
}
>
<RouterContext.Provider value={makePublicRouterInstance(router)}>
<RouterContext.Provider value={publicRouterInstance}>
<HeadManagerContext.Provider value={headManager}>
{children}
</HeadManagerContext.Provider>
Expand Down