diff --git a/.changeset/mean-pugs-play.md b/.changeset/mean-pugs-play.md new file mode 100644 index 0000000000..a159915353 --- /dev/null +++ b/.changeset/mean-pugs-play.md @@ -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`. diff --git a/packages/react-router/src/Match.tsx b/packages/react-router/src/Match.tsx index 6bca031a4d..8fd826affb 100644 --- a/packages/react-router/src/Match.tsx +++ b/packages/react-router/src/Match.tsx @@ -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 | 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. +function selectMatchFields( + router: ReturnType, + 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, + 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 + return ( + + ) } const matchStore = router.stores.getMatchStore(routeId) // eslint-disable-next-line react-hooks/rules-of-hooks - const match = useStore(matchStore, (value) => value) - return + const selection = useStore( + matchStore, + (match) => selectMatchFields(router, routeId, match), + matchSelectionEqual, + ) + return }) +// `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 resetKey} {...props} /> +} + function MatchView({ router, - match, + routeId, + selection, }: { router: ReturnType - 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 ( - + 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) }} > { - 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 ? ( - + ) : ( - + )} @@ -150,48 +265,35 @@ 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 ? : - }, [key, route.options.component, router.options.defaultComponent]) + return Comp ? : + }, [remountKey, route.options.component, router.options.defaultComponent]) - 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 ?? @@ -199,7 +301,7 @@ export const MatchInner = React.memo(function MatchInnerImpl({ ErrorComponent return ( ) } - throw match.error + throw error } return out diff --git a/packages/react-router/tests/match-rerender-probe.test.tsx b/packages/react-router/tests/match-rerender-probe.test.tsx new file mode 100644 index 0000000000..82b88601a7 --- /dev/null +++ b/packages/react-router/tests/match-rerender-probe.test.tsx @@ -0,0 +1,412 @@ +import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest' +import { act, cleanup, render, screen } from '@testing-library/react' +import * as React from 'react' +import type * as ReactNS from 'react' + +// --------------------------------------------------------------------------- +// Render counter: patch React.memo so every `React.memo(function XImpl(){})` +// component in the router source is wrapped with a counter keyed on the inner +// function name. This is applied identically to the baseline and the patched +// build, so the counts are directly comparable. +// --------------------------------------------------------------------------- +const renderCounts: Record = {} + +vi.mock('react', async (importOriginal) => { + const actual = await importOriginal() + const origMemo = actual.memo + const memo = ((fn: any, cmp: any) => { + if (typeof fn !== 'function' || !fn.name) return origMemo(fn, cmp) + const name = fn.name + const wrapper = (props: any, ref: any) => { + renderCounts[name] = (renderCounts[name] ?? 0) + 1 + return fn(props, ref) + } + Object.defineProperty(wrapper, 'name', { value: name }) + return origMemo(wrapper as any, cmp) + }) as typeof actual.memo + return { ...actual, memo, default: { ...(actual as any), memo } } +}) + +const { + Link, + Outlet, + RouterProvider, + createMemoryHistory, + createRootRoute, + createRoute, + createRouter, + useRouterState, +} = await import('../src') + +function resetCounts() { + for (const k of Object.keys(renderCounts)) delete renderCounts[k] +} + +// naive structural equality used only for reporting (does NOT ignore undefined) +function sameDeep(a: any, b: any, seen = new Set()): boolean { + if (Object.is(a, b)) return true + if (typeof a !== 'object' || typeof b !== 'object' || !a || !b) return false + if (seen.has(a)) return true + seen.add(a) + const ka = Object.keys(a) + const kb = Object.keys(b) + if (ka.length !== kb.length) return false + return ka.every((k) => sameDeep(a[k], b[k], seen)) +} + +function createTestRouter() { + const rootRoute = createRootRoute({ + component: function RootComp() { + // A root that genuinely re-renders on every navigation. + const href = useRouterState({ select: (s) => s.location.href }) + return ( +
+ {href} + +
+ ) + }, + }) + + const sectionRoute = createRoute({ + getParentRoute: () => rootRoute, + path: 'section', + validateSearch: (search: Record) => ({ + tab: (search.tab as string) ?? 'all', + }), + loader: () => ({ section: true }), + component: () => , + }) + + const listRoute = createRoute({ + getParentRoute: () => sectionRoute, + path: 'list', + loader: () => ({ list: [1, 2, 3] }), + component: () => ( +
+ {/* inline literals on purpose: fresh object identity every render */} + + go a + + + go b + + +
+ ), + }) + + const leafA = createRoute({ + getParentRoute: () => listRoute, + path: 'a', + component: () =>
leaf-a
, + }) + const leafB = createRoute({ + getParentRoute: () => listRoute, + path: 'b', + component: () =>
leaf-b
, + }) + + const routeTree = rootRoute.addChildren([ + sectionRoute.addChildren([listRoute.addChildren([leafA, leafB])]), + ]) + + return createRouter({ + routeTree, + basepath: '/app', + history: createMemoryHistory({ + initialEntries: ['/app/section/list/a?tab=all'], + }), + }) +} + +type Probe = { + publishes: Array<{ + routeId: string + deepEqual: boolean + changedKeys: Array + }> +} + +function instrument(router: any): Probe { + const probe: Probe = { publishes: [] } + for (const [routeId, store] of router.stores.byRoute as Map) { + let prev = store.get() + store.subscribe(() => { + const next = store.get() + if (!prev || !next) { + prev = next + return + } + const changedKeys = [ + ...new Set([...Object.keys(prev), ...Object.keys(next)]), + ] + .filter((k) => !Object.is(prev[k], next[k])) + .map((k) => + sameDeep(prev[k], next[k]) ? `${k}(identity-only)` : `${k}(value)`, + ) + probe.publishes.push({ + routeId, + deepEqual: sameDeep(prev, next), + changedKeys, + }) + prev = next + }) + } + return probe +} + +describe('Match re-render churn probe', () => { + afterEach(() => { + cleanup() + resetCounts() + }) + + test('sibling navigation /section/list/a -> /section/list/b', async () => { + const router = createTestRouter() + await act(() => router.load()) + render() + await act(() => new Promise((r) => setTimeout(r, 0))) + expect(screen.getByTestId('leaf')).toHaveTextContent('leaf-a') + + const stayingIds = [ + ...(router.stores.byRoute as Map).keys(), + ].filter((id) => id !== '/section/list/a') + const probe = instrument(router) + resetCounts() + + await act(async () => { + await router.navigate({ to: '/section/list/b', search: { tab: 'all' } }) + }) + await act(() => new Promise((r) => setTimeout(r, 0))) + expect(screen.getByTestId('leaf')).toHaveTextContent('leaf-b') + + const report = { + MatchImpl: renderCounts.MatchImpl ?? 0, + MatchInnerImpl: renderCounts.MatchInnerImpl ?? 0, + OutletImpl: renderCounts.OutletImpl ?? 0, + LinkImpl: renderCounts.LinkComponentImpl ?? renderCounts.LinkImpl ?? 0, + stayingMatchPublishes: probe.publishes.filter((p) => + stayingIds.includes(p.routeId), + ).length, + stayingMatchPublishesDeepEqual: probe.publishes.filter( + (p) => stayingIds.includes(p.routeId) && p.deepEqual, + ).length, + publishDetail: probe.publishes.map((p) => ({ + routeId: p.routeId, + changedKeys: p.changedKeys, + })), + allRenderCounts: { ...renderCounts }, + } + + console.log('PROBE_SIBLING_NAV ' + JSON.stringify(report, null, 2)) + + // Three routes (root, /section, /section/list) persist across this + // navigation and render nothing that changed. `Match` is keyed by routeId + // on main, so the leaf route's `Match` swaps its store contents in place + // rather than mounting a new one -- one render for the leaf, none for the + // three that stayed. + expect(report.MatchImpl).toBe(1) + expect(report.MatchInnerImpl).toBe(1) + }) + + test('search-only navigation ?tab=all -> ?tab=mine', async () => { + const router = createTestRouter() + await act(() => router.load()) + render() + await act(() => new Promise((r) => setTimeout(r, 0))) + + const probe = instrument(router) + resetCounts() + + await act(async () => { + await router.navigate({ to: '/section/list/a', search: { tab: 'mine' } }) + }) + await act(() => new Promise((r) => setTimeout(r, 0))) + + // Confirm the search navigation actually landed before asserting that it + // caused no re-renders -- otherwise the zero-render assertions would pass + // even if the navigation had silently done nothing. + expect(screen.getByTestId('href')).toHaveTextContent('tab=mine') + + const report = { + MatchImpl: renderCounts.MatchImpl ?? 0, + MatchInnerImpl: renderCounts.MatchInnerImpl ?? 0, + OutletImpl: renderCounts.OutletImpl ?? 0, + publishes: probe.publishes.length, + publishDetail: probe.publishes.map((p) => ({ + routeId: p.routeId, + changedKeys: p.changedKeys, + })), + } + + console.log('PROBE_SEARCH_NAV ' + JSON.stringify(report, null, 2)) + + // No route enters or leaves, and nothing Match/MatchInner render depends + // on changes -- search reaches route components through useSearch. + expect(report.MatchImpl).toBe(0) + expect(report.MatchInnerImpl).toBe(0) + }) + + test('same-route re-navigation (no-op href change)', async () => { + const router = createTestRouter() + await act(() => router.load()) + render() + await act(() => new Promise((r) => setTimeout(r, 0))) + + const probe = instrument(router) + resetCounts() + + await act(async () => { + await router.navigate({ to: '/section/list/b', search: { tab: 'all' } }) + }) + await act(async () => { + await router.navigate({ to: '/section/list/a', search: { tab: 'all' } }) + }) + await act(() => new Promise((r) => setTimeout(r, 0))) + + const report = { + MatchImpl: renderCounts.MatchImpl ?? 0, + MatchInnerImpl: renderCounts.MatchInnerImpl ?? 0, + OutletImpl: renderCounts.OutletImpl ?? 0, + publishes: probe.publishes.length, + publishDetail: probe.publishes.map((p) => ({ + routeId: p.routeId, + changedKeys: p.changedKeys, + })), + } + + console.log('PROBE_TWO_NAVS ' + JSON.stringify(report, null, 2)) + + // One leaf swap per navigation, nothing else. + expect(report.MatchImpl).toBe(2) + expect(report.MatchInnerImpl).toBe(2) + }) +}) + +describe('error boundary reset semantics', () => { + beforeEach(() => { + vi.spyOn(console, 'error').mockImplementation(() => {}) + vi.spyOn(console, 'warn').mockImplementation(() => {}) + }) + afterEach(() => { + cleanup() + resetCounts() + vi.restoreAllMocks() + }) + + test('a layout that throws during render resets its boundary on the next navigation', async () => { + // The layout route stays mounted across the sibling navigation, so the + // ONLY thing that can clear its CatchBoundary is a resetKey change. + let shouldThrow = true + + const rootRoute = createRootRoute({ component: () => }) + const layoutRoute = createRoute({ + getParentRoute: () => rootRoute, + path: 'layout', + errorComponent: () =>
boundary
, + component: function LayoutComp() { + if (shouldThrow) throw new Error('boom') + return ( +
+ layout-ok + +
+ ) + }, + }) + const a = createRoute({ + getParentRoute: () => layoutRoute, + path: 'a', + component: () =>
a
, + }) + const b = createRoute({ + getParentRoute: () => layoutRoute, + path: 'b', + component: () =>
b
, + }) + + const router = createRouter({ + routeTree: rootRoute.addChildren([layoutRoute.addChildren([a, b])]), + history: createMemoryHistory({ initialEntries: ['/layout/a'] }), + }) + + await act(() => router.load()) + render() + await act(() => new Promise((r) => setTimeout(r, 0))) + + expect(screen.getByTestId('boundary')).toBeInTheDocument() + + shouldThrow = false + await act(async () => { + await router.navigate({ to: '/layout/b' }) + }) + await act(() => new Promise((r) => setTimeout(r, 0))) + + expect(screen.queryByTestId('boundary')).not.toBeInTheDocument() + expect(screen.getByTestId('layout')).toHaveTextContent('layout-ok') + expect(screen.getByTestId('leaf')).toHaveTextContent('b') + }) + + test('error thrown by a staying layout after the last commit still resets on the next navigation', async () => { + // Timing edge: the component renders fine on the first commit, then a + // later state change makes it throw *after* the match store last changed. + // The next navigation must still reset the boundary. + let shouldThrow = false + let forceRerender: (() => void) | undefined + + const rootRoute = createRootRoute({ component: () => }) + const layoutRoute = createRoute({ + getParentRoute: () => rootRoute, + path: 'layout', + errorComponent: () =>
boundary
, + component: function LayoutComp() { + const [, setTick] = React.useState(0) + forceRerender = () => setTick((t) => t + 1) + if (shouldThrow) throw new Error('late boom') + return ( +
+ layout-ok + +
+ ) + }, + }) + const a = createRoute({ + getParentRoute: () => layoutRoute, + path: 'a', + component: () =>
a
, + }) + const b = createRoute({ + getParentRoute: () => layoutRoute, + path: 'b', + component: () =>
b
, + }) + + const router = createRouter({ + routeTree: rootRoute.addChildren([layoutRoute.addChildren([a, b])]), + history: createMemoryHistory({ initialEntries: ['/layout/a'] }), + }) + + await act(() => router.load()) + render() + await act(() => new Promise((r) => setTimeout(r, 0))) + expect(screen.getByTestId('layout')).toBeInTheDocument() + + // trip the error well after the last match-store change + shouldThrow = true + act(() => { + forceRerender!() + }) + expect(screen.getByTestId('boundary')).toBeInTheDocument() + + shouldThrow = false + await act(async () => { + await router.navigate({ to: '/layout/b' }) + }) + await act(() => new Promise((r) => setTimeout(r, 0))) + + expect(screen.queryByTestId('boundary')).not.toBeInTheDocument() + expect(screen.getByTestId('leaf')).toHaveTextContent('b') + }) +})