-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
perf(react-router): stop persisted matches re-rendering on every navigation #7990
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
base: main
Are you sure you want to change the base?
Changes from all commits
b893a2d
019b570
0a24b3d
9b6318c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@tanstack/react-router': patch | ||
| --- | ||
|
|
||
| Stop persisted route matches from re-rendering on every navigation. `Match` subscribed to its match store by identity, and `buildMatches` re-mints every staying match on every navigation, so each mounted route re-rendered its `MatchView`/`Suspense`/`CatchBoundary`/`CatchNotFound`/`MatchInner` chain even when nothing it renders had changed. `Match` now selects only the fields its subtree renders, and the per-navigation identity that resets `CatchBoundary` is observed in a wrapper that is only mounted for routes that actually have an `errorComponent`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import { SafeFragment } from './SafeFragment' | |
| import { renderRouteNotFound } from './renderRouteNotFound' | ||
| import { ScrollRestoration } from './scroll-restoration' | ||
| import { ClientOnly } from './ClientOnly' | ||
| import type { ErrorRouteComponent } from './route' | ||
| import type { | ||
| AnyRoute, | ||
| AnyRouteMatch, | ||
|
|
@@ -37,6 +38,72 @@ const outletMatchSelectionEqual = ( | |
| b: OutletMatchSelection, | ||
| ) => a[0] === b[0] && a[1] === b[1] | ||
|
|
||
| type MatchSelection = [ | ||
| matchId: string | undefined, | ||
| ssr: boolean | 'data-only' | undefined, | ||
| status: AnyRouteMatch['status'] | undefined, | ||
| error: unknown, | ||
| remountKey: string | undefined, | ||
| lazy: LazyRouteState, | ||
| ] | ||
|
|
||
| // `_lazy` is marked `@internal`, so it is stripped from the published | ||
| // declarations router-core's consumers compile against. | ||
| type LazyRouteState = Promise<void> | true | undefined | ||
|
|
||
| const matchSelectionEqual = (a: MatchSelection, b: MatchSelection) => | ||
| a[0] === b[0] && | ||
| a[1] === b[1] && | ||
| a[2] === b[2] && | ||
| a[3] === b[3] && | ||
| a[4] === b[4] && | ||
| a[5] === b[5] | ||
|
|
||
| const emptyMatchSelection: MatchSelection = [ | ||
| undefined, | ||
| undefined, | ||
| undefined, | ||
| undefined, | ||
| undefined, | ||
| undefined, | ||
| ] | ||
|
|
||
| // `buildMatches` re-mints every staying match on every navigation (a fresh | ||
| // object with a fresh `_strictSearch`, `context` and `abortController`, and an | ||
| // updated `cause`), so a match store always republishes with a new identity. | ||
| // Selecting only the fields this subtree renders lets a staying route bail out. | ||
| // `loaderDeps`/`_strictParams`/`_strictSearch` reach the output solely through | ||
| // the remount key, so the key is computed here and compared as a string. | ||
| // `route._lazy` is selected too: a lazy route's options are assigned onto the | ||
| // route in place, and a re-offered pending match is the only signal that the | ||
| // components this subtree renders have just been replaced. | ||
|
Comment on lines
+77
to
+79
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wouldn't something else also change at the same time when the components are replaced? like the |
||
| function selectMatchFields( | ||
| router: ReturnType<typeof useRouter>, | ||
| routeId: string, | ||
| match: AnyRouteMatch | undefined, | ||
| ): MatchSelection { | ||
| if (!match) return emptyMatchSelection | ||
|
|
||
| const route = router.routesById[routeId] as AnyRoute | ||
| const remountFn = | ||
| route.options.remountDeps ?? router.options.defaultRemountDeps | ||
| const remountDeps = remountFn?.({ | ||
| routeId, | ||
| loaderDeps: match.loaderDeps, | ||
| params: match._strictParams, | ||
| search: match._strictSearch, | ||
| }) | ||
|
|
||
| return [ | ||
| match.id, | ||
| match.ssr, | ||
| match.status, | ||
| match.error, | ||
|
Comment on lines
+98
to
+101
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not just return the |
||
| remountDeps ? JSON.stringify(remountDeps) : undefined, | ||
| (route as { _lazy?: LazyRouteState })._lazy, | ||
| ] | ||
| } | ||
|
|
||
| export const Match = React.memo(function MatchImpl({ | ||
| routeId, | ||
| }: { | ||
|
|
@@ -46,23 +113,61 @@ export const Match = React.memo(function MatchImpl({ | |
|
|
||
| if (isServer ?? router.isServer) { | ||
| const match = router.stores.byRoute.get(routeId)!.get()! | ||
| return <MatchView router={router} match={match} /> | ||
| return ( | ||
| <MatchView | ||
| router={router} | ||
| routeId={routeId} | ||
| selection={selectMatchFields(router, routeId, match)} | ||
| /> | ||
| ) | ||
| } | ||
|
|
||
| const matchStore = router.stores.getMatchStore(routeId) | ||
| // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| const match = useStore(matchStore, (value) => value) | ||
| return <MatchView router={router} match={match!} /> | ||
| const selection = useStore( | ||
| matchStore, | ||
| (match) => selectMatchFields(router, routeId, match), | ||
| matchSelectionEqual, | ||
| ) | ||
| return <MatchView router={router} routeId={routeId} selection={selection} /> | ||
| }) | ||
|
|
||
| // `CatchBoundary` resets its error whenever `getResetKey()` changes, and the | ||
| // match identity is what changes per navigation. Observing it here rather than | ||
| // in `Match` keeps that per-navigation update off every mounted route: only | ||
| // routes that actually have an `errorComponent` pay for it, and because | ||
| // `props.children` is the element `MatchView` already created, the subtree | ||
| // below bails out. | ||
| function ResettableCatchBoundary({ | ||
| routeId, | ||
| ...props | ||
| }: { | ||
| routeId: string | ||
| children: React.ReactNode | ||
| errorComponent?: ErrorRouteComponent | ||
| onCatch?: (error: Error, errorInfo: React.ErrorInfo) => void | ||
| }) { | ||
| const router = useRouter() | ||
| const resetKey = | ||
| (isServer ?? router.isServer) | ||
| ? router.stores.byRoute.get(routeId)!.get() | ||
| : // eslint-disable-next-line react-hooks/rules-of-hooks -- condition is static | ||
| useStore(router.stores.getMatchStore(routeId), (match) => match) | ||
|
|
||
| return <CatchBoundary getResetKey={() => resetKey} {...props} /> | ||
| } | ||
|
|
||
| function MatchView({ | ||
| router, | ||
| match, | ||
| routeId, | ||
| selection, | ||
| }: { | ||
| router: ReturnType<typeof useRouter> | ||
| match: AnyRouteMatch | ||
| routeId: string | ||
| selection: MatchSelection | ||
| }) { | ||
| const route: AnyRoute = router.routesById[match.routeId] | ||
| const [matchId, ssr, status, error, remountKey] = selection | ||
| const route: AnyRoute = router.routesById[routeId] | ||
|
|
||
| const pendingElement = renderPending(router, route) | ||
|
|
||
|
|
@@ -77,7 +182,7 @@ function MatchView({ | |
| router.options.notFoundRoute?.options.component) | ||
| : route.options.notFoundComponent | ||
|
|
||
| const resolvedNoSsr = match.ssr === false || match.ssr === 'data-only' | ||
| const resolvedNoSsr = ssr === false || ssr === 'data-only' | ||
| const ResolvedSuspenseBoundary = | ||
| (route.options.wrapInSuspense ?? | ||
| pendingElement ?? | ||
|
|
@@ -86,7 +191,7 @@ function MatchView({ | |
| : SafeFragment | ||
|
|
||
| const ResolvedCatchBoundary = routeErrorComponent | ||
| ? CatchBoundary | ||
| ? ResettableCatchBoundary | ||
| : SafeFragment | ||
|
|
||
| const ResolvedNotFoundBoundary = routeNotFoundComponent | ||
|
|
@@ -98,28 +203,28 @@ function MatchView({ | |
| : SafeFragment | ||
| return ( | ||
| <ShellComponent> | ||
| <matchContext.Provider value={match.routeId}> | ||
| <matchContext.Provider value={routeId}> | ||
| <ResolvedSuspenseBoundary fallback={pendingElement}> | ||
| <ResolvedCatchBoundary | ||
| getResetKey={() => match} | ||
| routeId={routeId} | ||
| errorComponent={routeErrorComponent as any} | ||
| onCatch={(error, errorInfo) => { | ||
| // Forward not found errors (we don't want to show the error component for these) | ||
| if (isNotFound(error)) { | ||
| error.routeId ??= match.routeId | ||
| error.routeId ??= routeId | ||
| throw error | ||
| } | ||
| if (process.env.NODE_ENV !== 'production') { | ||
| console.warn(`Warning: Error in route match: ${match.id}`) | ||
| console.warn(`Warning: Error in route match: ${matchId}`) | ||
| } | ||
| routeOnCatch?.(error, errorInfo) | ||
| }} | ||
| > | ||
| <ResolvedNotFoundBoundary | ||
| fallback={(error) => { | ||
| error.routeId ??= match.routeId | ||
| error.routeId ??= routeId | ||
|
|
||
| if (error.routeId !== match.routeId) { | ||
| if (error.routeId !== routeId) { | ||
| throw error | ||
| } | ||
|
|
||
|
|
@@ -131,10 +236,20 @@ function MatchView({ | |
| > | ||
| {resolvedNoSsr ? ( | ||
| <ClientOnly fallback={pendingElement}> | ||
| <MatchInner match={match} /> | ||
| <MatchInner | ||
| routeId={routeId} | ||
| status={status} | ||
| error={error} | ||
| remountKey={remountKey} | ||
| /> | ||
| </ClientOnly> | ||
| ) : ( | ||
| <MatchInner match={match} /> | ||
| <MatchInner | ||
| routeId={routeId} | ||
| status={status} | ||
| error={error} | ||
| remountKey={remountKey} | ||
| /> | ||
| )} | ||
| </ResolvedNotFoundBoundary> | ||
| </ResolvedCatchBoundary> | ||
|
|
@@ -150,64 +265,51 @@ function MatchView({ | |
| } | ||
|
|
||
| export const MatchInner = React.memo(function MatchInnerImpl({ | ||
| match, | ||
| routeId, | ||
| status, | ||
| error, | ||
| remountKey, | ||
| }: { | ||
| match: AnyRouteMatch | ||
| routeId: string | ||
| status: AnyRouteMatch['status'] | undefined | ||
| error: unknown | ||
| remountKey: string | undefined | ||
| }): any { | ||
| const router = useRouter() | ||
| const routeId = match.routeId | ||
| const route = router.routesById[routeId] as AnyRoute | ||
| const key = React.useMemo(() => { | ||
| const remountFn = | ||
| route.options.remountDeps ?? router.options.defaultRemountDeps | ||
| const remountDeps = remountFn?.({ | ||
| routeId, | ||
| loaderDeps: match.loaderDeps, | ||
| params: match._strictParams, | ||
| search: match._strictSearch, | ||
| }) | ||
| return remountDeps ? JSON.stringify(remountDeps) : undefined | ||
| }, [ | ||
| routeId, | ||
| match.loaderDeps, | ||
| match._strictParams, | ||
| match._strictSearch, | ||
| route.options.remountDeps, | ||
| router.options.defaultRemountDeps, | ||
| ]) | ||
| const out = React.useMemo(() => { | ||
| const Comp = route.options.component ?? router.options.defaultComponent | ||
| return Comp ? <Comp key={key} /> : <Outlet /> | ||
| }, [key, route.options.component, router.options.defaultComponent]) | ||
| return Comp ? <Comp key={remountKey} /> : <Outlet /> | ||
| }, [remountKey, route.options.component, router.options.defaultComponent]) | ||
|
Comment on lines
267
to
+283
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Description: Trace _lazy assignment and match status updates around lazy route loading.
set -euo pipefail
echo "=== _lazy assignments and reads across the repository ==="
rg -n -C4 '\b_lazy\b' --glob '!**/dist/**' --glob '!**/*.md'
echo "=== Lazy option assignment onto route.options ==="
ast-grep run --pattern 'Object.assign($ROUTE.options, $$$)' --lang typescript packages || true
echo "=== Status transitions written by the loader ==="
fd -t f 'load.ts' packages/router-core/src | xargs -r rg -n -C3 "status: '(pending|success|error|notFound)'"Repository: TanStack/router Length of output: 211 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Candidate Match.tsx ==="
fd -a 'Match\.tsx$' . | sed 's#^\./##'
echo "=== Locate selectMatchFields and MatchView/MatchInner ==="
rg -n "selectMatchFields|MatchView|MatchInner|function MatchInnerImpl|React\.memo\(|selection|_lazy|lazy:" --glob '!**/dist/**' --glob '!**/*.md' .
echo "=== List router-core src files ==="
fd -t f . packages/router-core/src | sed 's#^\./##' | head -200Repository: TanStack/router Length of output: 18822 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Match.tsx relevant sections ==="
cat -n packages/react-router/src/Match.tsx | sed -n '45,175p'
cat -n packages/react-router/src/Match.tsx | sed -n '180,315p'
echo "=== Load-client lazy implementation ==="
cat -n packages/router-core/src/load-client.ts | sed -n '1,115p'
cat -n packages/router-core/src/load-client.ts | sed -n '845,885p'
echo "=== Router store update / transaction application references ==="
rg -n -C5 "apply.*transaction|transactions|_tx|router\.state|status: 'pending'|status: 'success'|pending|success" packages/router-core/src/packages/react-router/src --glob '!**/dist/**'Repository: TanStack/router Length of output: 16667 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Match.tsx MatchInner tail ==="
cat -n packages/react-router/src/Match.tsx | sed -n '308,320p'
echo "=== Router transaction/update/status path ==="
rg -n -C4 "status: 'pending'|status: 'success'|replaceRouteChunk|loadRouteChunk|onReady:|options\[8\]|status =" packages/router-core/src packages/react-router/src --glob '!**/dist/**'
echo "=== Read-only behavioral probe: current _lazy selection and MatchInner props ==="
python3 - <<'PY'
from pathlib import Path
p = Path('packages/react-router/src/Match.tsx')
text = p.read_text()
checks = {
'MatchSelection_has_lazy_field': "lazy: LazyRouteState," in text and "a[5] === b[5]" in text,
'selectMatchFields_returns_lazy': " (route as { _lazy?: LazyRouteState })._lazy," in text,
'MatchView_destructures_lazy': "_lazy" in text.split("function MatchView")[1].split("const pendingElement")[0],
'MatchInner_renders_lazy_prop': "lazy={lazy}" in text,
'MatchInner_declares_lazy_prop': "lazy," in text.split("export const MatchInner")[1].split("): any {")[0],
'MatchInner useMemo uses lazy': "'lazy'" in text.split("export const MatchInner")[1] or '"lazy"' in text.split("export const MatchInner")[1] or "lazy," in text.split("export const MatchInner")[1] or " lazy" in text.split("export const MatchInner")[1],
}
for name, ok in checks.items():
print(f"{name}: {ok}")
PYRepository: TanStack/router Length of output: 47448 The
🤖 Prompt for AI Agents |
||
|
|
||
| if (match.status === 'pending') { | ||
| if (status === 'pending') { | ||
| if (router._tx) { | ||
| throw router._tx[5] | ||
| } | ||
| return renderPending(router, route) | ||
| } | ||
|
|
||
| if (match.status === 'notFound') { | ||
| return renderRouteNotFound(router, route, match.error) | ||
| if (status === 'notFound') { | ||
| return renderRouteNotFound(router, route, error) | ||
| } | ||
|
|
||
| if (match.status === 'error') { | ||
| if (status === 'error') { | ||
| if (isServer ?? router.isServer) { | ||
| const RouteErrorComponent = | ||
| (route.options.errorComponent ?? | ||
| router.options.defaultErrorComponent) || | ||
| ErrorComponent | ||
| return ( | ||
| <RouteErrorComponent | ||
| error={match.error as any} | ||
| error={error as any} | ||
| reset={undefined as any} | ||
| info={{ | ||
| componentStack: '', | ||
| }} | ||
| /> | ||
| ) | ||
| } | ||
| throw match.error | ||
| throw error | ||
| } | ||
|
|
||
| return out | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't just
[]do the same thing?