diff --git a/.changeset/fewer-store-updates.md b/.changeset/fewer-store-updates.md new file mode 100644 index 00000000000..9d1ca8d1b08 --- /dev/null +++ b/.changeset/fewer-store-updates.md @@ -0,0 +1,5 @@ +--- +'@tanstack/router-core': patch +--- + +Publish fewer router store updates during a navigation. Every synchronous frame between two `beforeLoad` awaits now publishes once, so ending one `beforeLoad` and starting the next hook or the loaders no longer produces separate `isFetching` updates. Loader starts for a whole lane publish together, and a superseding navigation clears the previous lane's fetching state in the same update that publishes its new location. `waitFor` no longer registers an abort listener for plain values. diff --git a/packages/react-router/tests/store-updates-during-navigation.bench.tsx b/packages/react-router/tests/store-updates-during-navigation.bench.tsx new file mode 100644 index 00000000000..eaa7d09f21d --- /dev/null +++ b/packages/react-router/tests/store-updates-during-navigation.bench.tsx @@ -0,0 +1,106 @@ +import { bench, describe, expect } from 'vitest' +import { render } from '@testing-library/react' +import { + Outlet, + RouterProvider, + createMemoryHistory, + createRootRoute, + createRoute, + createRouter, + useParams, + useRouterState, + useSearch, +} from '../src' + +// Navigation cost under store-subscriber fan-out. A retained layout mounts +// `subscribers` broad `useRouterState` consumers plus as many narrow +// `useSearch`/`useParams` consumers. Every navigation re-runs two retained +// synchronous `beforeLoad`s and a stale layout loader, so the router publishes +// fetching transitions on presented matches. All selections are stable, so the +// measured work is store propagation and selection rather than re-rendering. +async function setup(subscribers: number) { + let notifications = 0 + + const Broad = () => { + useRouterState({ select: (state) => state.location.pathname }) + return null + } + const Narrow = () => { + useSearch({ strict: false }) + useParams({ strict: false }) + return null + } + const Counter = () => { + useRouterState({ + select: () => { + notifications++ + }, + }) + return null + } + + const rootRoute = createRootRoute({ + beforeLoad: () => ({ root: true }), + component: () => ( + <> + + + + ), + }) + const layoutRoute = createRoute({ + getParentRoute: () => rootRoute, + id: 'layout', + beforeLoad: () => ({ layout: true }), + loader: () => ({ items: [] }), + staleTime: 0, + component: () => ( + <> + {Array.from({ length: subscribers }, (_, index) => ( + + ))} + {Array.from({ length: subscribers }, (_, index) => ( + + ))} + + + ), + }) + const pages = ['a', 'b'].map((path) => + createRoute({ + getParentRoute: () => layoutRoute, + path, + beforeLoad: () => ({ page: path }), + loader: () => path, + component: () =>

{path}

, + }), + ) + const router = createRouter({ + routeTree: rootRoute.addChildren([layoutRoute.addChildren(pages)]), + history: createMemoryHistory({ initialEntries: ['/a'] }), + }) + render() + await router.load() + + const lap = async () => { + await router.navigate({ to: '/b', replace: true }) + await router.navigate({ to: '/a', replace: true }) + } + await lap() + const before = notifications + await lap() + const perNavigation = (notifications - before) / 2 + expect(router.state.location.pathname).toBe('/a') + expect(perNavigation).toBeGreaterThan(0) + return { lap, perNavigation } +} + +for (const subscribers of [0, 50, 200]) { + const { lap, perNavigation } = await setup(subscribers) + describe(`${subscribers} broad + ${subscribers} narrow subscribers (${perNavigation} store updates per navigation)`, () => { + bench('two navigations with retained beforeLoads and a stale loader', lap, { + time: 1000, + warmupTime: 200, + }) + }) +} diff --git a/packages/react-router/tests/store-updates-during-navigation.test.tsx b/packages/react-router/tests/store-updates-during-navigation.test.tsx index 8a10e06339a..3a1c5bb81f1 100644 --- a/packages/react-router/tests/store-updates-during-navigation.test.tsx +++ b/packages/react-router/tests/store-updates-during-navigation.test.tsx @@ -10,19 +10,37 @@ import { Link, Outlet, RouterProvider, + createControlledPromise, createRootRoute, createRoute, createRouter, notFound, redirect, + useParams, useRouterState, + useSearch, } from '../src' +import type { AnyRouter, RouterState } from '../src' afterEach(() => { window.history.replaceState(null, 'root', '/') cleanup() }) +/** + * Describe a published router state by value, so the trace still explains a + * publication after later mutations of the same match objects. + */ +function describeState(state: RouterState) { + const matches = state.matches + .map( + (match) => + `${match.routeId}:${match.status}${match.isFetching ? `+${match.isFetching}` : ''}`, + ) + .join(' ') + return `${state.status} ${state.location.pathname} → ${state.resolvedLocation?.pathname} [${matches}]` +} + function setup({ beforeLoad, loader, @@ -32,6 +50,7 @@ function setup({ defaultPendingMs, defaultPendingMinMs, staleTime, + rootBeforeLoad, }: { beforeLoad?: () => any loader?: () => any @@ -41,16 +60,49 @@ function setup({ defaultPendingMs?: number defaultPendingMinMs?: number staleTime?: number + rootBeforeLoad?: () => any }) { - const select = vi.fn() + // One entry per store notification (plus one per render of the root). + const trace: Array = [] + const select = vi.fn((state: RouterState) => { + trace.push(describeState(state)) + }) + const rootRenders = vi.fn() + // A genuine whole-state consumer: renders when the router starts and stops + // loading, and never for intermediate publications. + const indicatorRenders = vi.fn() + const Indicator = () => { + indicatorRenders(useRouterState({ select: (state) => state.isLoading })) + return null + } + // An object-valued selection with structural sharing keeps its reference + // while the selected values are equal. + const sharedSelections: Array<{ pathname: string }> = [] + const Shared = () => { + sharedSelections.push( + useRouterState({ + select: (state) => ({ pathname: state.location.pathname }), + structuralSharing: true, + }), + ) + return null + } const rootRoute = createRootRoute({ + beforeLoad: rootBeforeLoad, component: function RootComponent() { useRouterState({ select }) + // Persistent narrow consumers: unchanged selections must not re-render. + useSearch({ strict: false }) + useParams({ strict: false }) + rootRenders() return ( <> + + Back Posts + Other ) @@ -92,7 +144,22 @@ function setup({ render() - return { select, router } + return { + select, + router, + trace, + rootRenders, + indicatorRenders, + sharedSelections, + } +} + +/** Wait until a navigation has been acknowledged and nothing else publishes. */ +async function settled(router: AnyRouter, pathname: string) { + await waitFor(() => { + expect(router.state.status).toBe('idle') + expect(router.state.resolvedLocation?.pathname).toBe(pathname) + }) } async function back() { @@ -136,7 +203,10 @@ describe("Store doesn't update *too many* times during navigation", () => { // This number should be as small as possible to minimize the amount of work // that needs to be done during a navigation. // Any change that increases this number should be investigated. - expect(updates).toBe(7) + // 1. location + status, 2. pending placeholder after pendingMs, + // 3. beforeLoad → loader fetching transition (one synchronous frame), + // 4. loader settled, 5. matches committed, 6. resolvedLocation + status. + expect(updates).toBe(6) }) test('redirection in preload', async () => { @@ -291,3 +361,234 @@ describe("Store doesn't update *too many* times during navigation", () => { expect(updates).toBe(0) }) }) + +describe('every publication during a navigation is explained', () => { + const start = 'pending /posts → / [__root__:success /:success]' + const committed = 'pending /posts → / [__root__:success /posts:success]' + const resolved = 'idle /posts → /posts [__root__:success /posts:success]' + const placeholder = (fetching: '' | '+beforeLoad' | '+loader') => + `pending /posts → / [__root__:success /posts:pending${fetching}]` + + async function navigate({ + router, + trace, + indicatorRenders, + sharedSelections, + }: ReturnType) { + await settled(router, '/') + const from = trace.length + indicatorRenders.mockClear() + sharedSelections.length = 0 + fireEvent.click(screen.getByRole('link', { name: 'Posts' })) + return () => trace.slice(from) + } + + async function finish(params: ReturnType) { + await screen.findByRole('heading', { name: 'Posts Title' }) + await settled(params.router, '/posts') + const published = params.trace.length + // Nothing else may publish once the navigation is resolved. + await new Promise((resolve) => setTimeout(resolve, 30)) + expect(params.trace.length).toBe(published) + expect(params.rootRenders).toHaveBeenCalledTimes(1) + // Whole-state consumers only re-render when their selection changes: + // the indicator once for pending and once for idle, the shared object + // once for the new pathname. + expect(params.indicatorRenders.mock.calls).toEqual([[true], [false]]) + expect(params.sharedSelections).toEqual([{ pathname: '/posts' }]) + } + + test('no fallback: hooks settle before pendingMs', async () => { + const beforeLoad = createControlledPromise() + const loader = createControlledPromise() + const params = setup({ + beforeLoad: () => beforeLoad, + loader: () => loader, + defaultPendingMs: 1000, + defaultPendingMinMs: 0, + }) + + const published = await navigate(params) + beforeLoad.resolve() + loader.resolve() + await finish(params) + + // Nothing is presented while the hooks run, so their fetching transitions + // never publish: location, matches, resolution. + expect(published()).toEqual([start, committed, resolved]) + }) + + test('fallback during beforeLoad', async () => { + const beforeLoad = createControlledPromise() + const loader = createControlledPromise() + const params = setup({ + beforeLoad: () => beforeLoad, + loader: () => loader, + defaultPendingMs: 20, + defaultPendingMinMs: 0, + }) + + const published = await navigate(params) + await screen.findByText('Loading...') + expect(published()).toEqual([start, placeholder('+beforeLoad')]) + + beforeLoad.resolve() + // Ending beforeLoad and starting the loader is one synchronous frame. + await waitFor(() => + expect(published()).toEqual([ + start, + placeholder('+beforeLoad'), + placeholder('+loader'), + ]), + ) + + loader.resolve() + await finish(params) + expect(published()).toEqual([ + start, + placeholder('+beforeLoad'), + placeholder('+loader'), + placeholder(''), + committed, + resolved, + ]) + }) + + test('fallback during the loader', async () => { + const beforeLoad = createControlledPromise() + const loader = createControlledPromise() + const params = setup({ + beforeLoad: () => beforeLoad, + loader: () => loader, + defaultPendingMs: 20, + defaultPendingMinMs: 0, + }) + + const published = await navigate(params) + beforeLoad.resolve() + await screen.findByText('Loading...') + // The placeholder is revealed with the loader already running. + expect(published()).toEqual([start, placeholder('+loader')]) + + loader.resolve() + await finish(params) + expect(published()).toEqual([ + start, + placeholder('+loader'), + placeholder(''), + committed, + resolved, + ]) + }) + + test('ready before the pending minimum expires', async () => { + const loader = createControlledPromise() + const params = setup({ + loader: () => loader, + defaultPendingMs: 20, + defaultPendingMinMs: 200, + }) + + const published = await navigate(params) + await screen.findByText('Loading...') + loader.resolve() + // The loader has settled but the fallback must stay visible: that is a + // real presentation delay, so fetching ends before the matches commit. + await waitFor(() => + expect(published()).toEqual([ + start, + placeholder('+loader'), + placeholder(''), + ]), + ) + expect(screen.getByText('Loading...')).toBeInTheDocument() + + await finish(params) + expect(published()).toEqual([ + start, + placeholder('+loader'), + placeholder(''), + committed, + resolved, + ]) + }) + + test('a replacement while the fallback is visible resets the placeholder with its first publication', async () => { + const loader = createControlledPromise() + const params = setup({ + loader: () => loader, + defaultPendingMs: 20, + defaultPendingMinMs: 0, + }) + const { router, trace } = params + + const published = await navigate(params) + await screen.findByText('Loading...') + expect(published()).toEqual([start, placeholder('+loader')]) + + fireEvent.click(screen.getByRole('link', { name: 'Other' })) + await screen.findByRole('heading', { name: 'Other Title' }) + await settled(router, '/other') + const count = trace.length + loader.resolve() + await new Promise((resolve) => setTimeout(resolve, 30)) + expect(trace.length).toBe(count) + + expect(published()).toEqual([ + start, + placeholder('+loader'), + // The visible placeholder stops fetching in the same update that + // publishes the replacement location; the abandoned loader never + // publishes again. + 'pending /other → / [__root__:success /posts:pending]', + 'pending /other → / [__root__:success /other:success]', + 'idle /other → /other [__root__:success /other:success]', + ]) + expect(params.rootRenders).toHaveBeenCalledTimes(1) + }) + + test('a superseding navigation resets fetching state in its first publication', async () => { + const rootGate = createControlledPromise() + let rootCalls = 0 + const params = setup({ + // The presented root re-runs beforeLoad for every navigation; only the + // superseded navigation blocks on it. + rootBeforeLoad: () => (++rootCalls === 2 ? rootGate : undefined), + defaultPendingMs: 1000, + }) + const { router, trace } = params + + await settled(router, '/') + const from = trace.length + fireEvent.click(screen.getByRole('link', { name: 'Posts' })) + await waitFor(() => + expect(trace.slice(from)).toEqual([ + start, + 'pending /posts → / [__root__:success+beforeLoad /:success]', + ]), + ) + + fireEvent.click(screen.getByRole('link', { name: 'Other' })) + await screen.findByRole('heading', { name: 'Other Title' }) + await settled(router, '/other') + const published = trace.length + rootGate.resolve() + // The superseded lane settles as canceled and must not publish anything. + await new Promise((resolve) => setTimeout(resolve, 30)) + expect(trace.length).toBe(published) + + expect(trace.slice(from)).toEqual([ + start, + 'pending /posts → / [__root__:success+beforeLoad /:success]', + // Clearing the superseded lane's fetching state and publishing the new + // location is one update. + 'pending /other → / [__root__:success /:success]', + // The new lane runs the root beforeLoad again (synchronously this time). + 'pending /other → / [__root__:success+beforeLoad /:success]', + 'pending /other → / [__root__:success /:success]', + 'pending /other → / [__root__:success /other:success]', + 'idle /other → /other [__root__:success /other:success]', + ]) + expect(params.rootRenders).toHaveBeenCalledTimes(1) + }) +}) diff --git a/packages/router-core/src/load-client.ts b/packages/router-core/src/load-client.ts index 1994eb2c371..60f16f8ce21 100644 --- a/packages/router-core/src/load-client.ts +++ b/packages/router-core/src/load-client.ts @@ -193,6 +193,9 @@ type LoaderOutcome = NonRedirectOutcome | RedirectOutcome type IndexedOutcome = [index: number, outcome: LoaderOutcome, boundary?: number] +/** A settled lane failure, a completed frame, or the frame after the next await. */ +type LaneStep = IndexedOutcome | undefined | Promise + export type LoaderFlight = [ outcome: Promise, controller: AbortController, @@ -288,6 +291,12 @@ export function waitFor( return Promise.race([Promise.reject(signal), value]) } return new Promise((resolve, reject) => { + // A plain value cannot be interrupted, so it needs no abort listener. A + // throwing `then` getter rejects through the executor. + if (typeof (value as any)?.then !== 'function') { + resolve(value as T) + return + } const abort = () => reject(signal) signal.addEventListener('abort', abort, { once: true }) Promise.resolve(value) @@ -354,122 +363,145 @@ function normalizeLaneError( ) } -async function contextualize( +function contextualize( router: AnyRouter, lane: MatchedLane, options: ExecuteLaneOptions, end: number, planSuccessfulLane: () => void, retainedEnd: number, -): Promise { +): LaneStep { const [location, matches] = lane - const signal = options[0 /* controller */].signal + const controller = options[0 /* controller */] + const signal = controller.signal const preload = !!options[3 /* preload */] - for (let index = options[6 /* resolvedPrefix */] ?? 0; index < end; index++) { - const match = matches[index]! - const route = getRoute(router, match) + let index = options[6 /* resolvedPrefix */] ?? 0 + // Everything between two beforeLoad awaits runs synchronously, so a single + // batch publishes the frame's fetching transitions, pending presentation, + // and planned loaders as one store update. + const frame = (): LaneStep => { + for (; index < end; index++) { + const match = matches[index]! + const route = getRoute(router, match) - match.abortController = options[0 /* controller */] - // Contextualization is serial, so the previous match already contains the - // complete parent context for this route. - const parentContext = - matches[index - 1]?.context ?? router.options.context ?? {} - const common = { - params: match.params, - location, - navigate: (opts: any) => - router.navigate({ - ...opts, - _fromLocation: location, - }), - buildLocation: router.buildLocation, - cause: preload ? ('preload' as const) : match.cause, - abortController: options[0 /* controller */], - preload, - matches, - routeId: route.id, - } - try { - // Reuse the route's cached contribution while rebuilding its inheritance. - const routeContext = (match._ctx ||= route.options.context - ? route.options.context({ - ...common, - deps: match.loaderDeps, - context: parentContext, - } satisfies RouteContextOptions) || {} - : undefined) - match.context = { - ...parentContext, - ...routeContext, + match.abortController = controller + // Contextualization is serial, so the previous match already contains + // the complete parent context for this route. + const parentContext = + matches[index - 1]?.context ?? router.options.context ?? {} + const common = { + params: match.params, + location, + navigate: (opts: any) => + router.navigate({ + ...opts, + _fromLocation: location, + }), + buildLocation: router.buildLocation, + cause: preload ? ('preload' as const) : match.cause, + abortController: controller, + preload, + matches, + routeId: route.id, + } + try { + // Reuse the route's cached contribution while rebuilding its + // inheritance. + const routeContext = (match._ctx ||= route.options.context + ? route.options.context({ + ...common, + deps: match.loaderDeps, + context: parentContext, + } satisfies RouteContextOptions) || {} + : undefined) + match.context = { + ...parentContext, + ...routeContext, + } + } catch (cause) { + releaseFlight(router, match) + return [index, normalizeLaneError(router, lane, route, cause, options)] } - } catch (cause) { - releaseFlight(router, match) - return [index, normalizeLaneError(router, lane, route, cause, options)] - } - if (signal.aborted) { - return [index, CANCELED_OUTCOME] - } - const validationError = match.paramsError ?? match.searchError - if (validationError !== undefined) { - releaseFlight(router, match) - return [ - index, - normalizeLaneError(router, lane, route, validationError, options), - ] - } - const beforeLoad = route.options.beforeLoad - if (!beforeLoad) { - continue - } - - const previousStatus = match.status - if (index >= retainedEnd) { - match.status = 'pending' - options[7 /* onReady */]?.() - } - try { - setFetching(router, match, 'beforeLoad', options[0 /* controller */]) - const value = beforeLoad({ - ...common, - search: match.search, - context: match.context, - ...router.options.additionalContext, - }) - // Always await to give a queued replacement navigation one microtask to - // kick in before checking cancellation, even for synchronous context. - const result = await (typeof value?.then === 'function' - ? waitFor(value, signal) - : value) if (signal.aborted) { return [index, CANCELED_OUTCOME] } - const outcome = materializeRedirect( - router, - lane, - route, - normalize(result, false, route.id), - options, - ) - if (outcome[0 /* kind */] !== SUCCESS) { + const validationError = match.paramsError ?? match.searchError + if (validationError !== undefined) { releaseFlight(router, match) - return [index, outcome] + return [ + index, + normalizeLaneError(router, lane, route, validationError, options), + ] } - match.context = { - ...match.context, - ...result, + const beforeLoad = route.options.beforeLoad + if (!beforeLoad) { + continue } - } catch (cause) { - releaseFlight(router, match) - return [index, normalizeLaneError(router, lane, route, cause, options)] - } finally { - match.status = previousStatus - setFetching(router, match, false, options[0 /* controller */]) + + const previousStatus = match.status + if (index >= retainedEnd) { + match.status = 'pending' + options[7 /* onReady */]?.() + } + setFetching(router, match, 'beforeLoad', controller) + // Applies the settled beforeLoad and continues the lane in one frame. + const settle = (value: any, rejected?: boolean): LaneStep => { + let next: LaneStep + router.batch(() => { + // The hook has ended, so `onError` and redirect resolution observe the + // restored status and cleared fetching state. + match.status = previousStatus + setFetching(router, match, false, controller) + const outcome = signal.aborted + ? CANCELED_OUTCOME + : materializeRedirect( + router, + lane, + route, + rejected + ? normalizeError(route, value) + : normalize(value, false, route.id), + options, + ) + if (outcome[0 /* kind */] === SUCCESS) { + match.context = { + ...match.context, + ...value, + } + index++ + next = frame() + } else { + // Releasing after a cancellation is a no-op: the superseding load + // already detached this lane's flights. + releaseFlight(router, match) + next = [index, outcome] + } + }) + return next + } + let value: any + try { + value = beforeLoad({ + ...common, + search: match.search, + context: match.context, + ...router.options.additionalContext, + }) + } catch (cause) { + return settle(cause, true) + } + // Always await, even a synchronous value, to give a queued replacement + // navigation one microtask to kick in before cancellation is checked. + return waitFor(value, signal).then(settle, (cause) => settle(cause, true)) } + + // Let a synchronous lane claim predecessor flights before this frame + // yields. + planSuccessfulLane() + return } - // Let a synchronous lane claim predecessor flights before this frame yields. - planSuccessfulLane() - return + return frame() } function releaseOwnedFlight( @@ -1345,22 +1377,24 @@ async function executeClientLane( let semanticParent = start ? Promise.resolve(matches[start - 1] as WorkMatch) : undefined - const planSuccessfulLane = () => { - for (let index = start; index < end; index++) { - if (signal.aborted) { - break + // Loader starts and pending presentation publish as one store update. + const planSuccessfulLane = () => + router.batch(() => { + for (let index = start; index < end; index++) { + if (signal.aborted) { + break + } + semanticParent = createLoaderTask( + router, + matched as ContextualizedLane, + index, + tasks, + semanticParent, + options, + retainedEnd, + ) } - semanticParent = createLoaderTask( - router, - matched as ContextualizedLane, - index, - tasks, - semanticParent, - options, - retainedEnd, - ) - } - } + }) // From here on `matched` is contextualized: `contextualize` communicates // through mutation plus a failure return, so the phase brand is asserted at // the two use sites below rather than granted by a (byte-costing) return. @@ -2012,14 +2046,6 @@ export async function loadClientRoute( } router._tx = tx if (previousOwner) { - for (const match of router.stores.matches.get() as Array) { - if (router._tx !== tx) { - break - } - if (match.isFetching) { - setFetching(router, match, false) - } - } previousOwner[0 /* controller */].abort() transferMatchResources( router, @@ -2036,6 +2062,13 @@ export async function loadClientRoute( return } router.batch(() => { + // A superseded lane can no longer publish, so clear the fetching state it + // left on the still-presented matches together with the new status. + for (const match of router.stores.matches.get() as Array) { + if (match.isFetching) { + setFetching(router, match, false) + } + } router.stores.status.set('pending') router.stores.location.set(location) }) diff --git a/packages/router-core/tests/load-client-wait-for.test.ts b/packages/router-core/tests/load-client-wait-for.test.ts index 9ae49b89bd9..b75e89d6ad7 100644 --- a/packages/router-core/tests/load-client-wait-for.test.ts +++ b/packages/router-core/tests/load-client-wait-for.test.ts @@ -49,7 +49,7 @@ describe('waitFor', () => { ) }) - test('removes its abort listener when reading a thenable throws', async () => { + test('rejects without an abort listener when reading a thenable throws', async () => { const controller = new AbortController() const add = vi.spyOn(controller.signal, 'addEventListener') const remove = vi.spyOn(controller.signal, 'removeEventListener') @@ -63,11 +63,23 @@ describe('waitFor', () => { await expect(waitFor(value, controller.signal)).rejects.toBe(error) await new Promise((resolve) => setTimeout(resolve, 0)) - expect(add).toHaveBeenCalledOnce() - expect(remove).toHaveBeenCalledExactlyOnceWith( - 'abort', - add.mock.calls[0]![1], - ) + expect(add).not.toHaveBeenCalled() + expect(remove).not.toHaveBeenCalled() + }) + + test('resolves a plain value without registering an abort listener', async () => { + const controller = new AbortController() + const add = vi.spyOn(controller.signal, 'addEventListener') + + const result = waitFor(42, controller.signal) + // Still asynchronous: a queued replacement navigation gets a microtask. + let settled = false + void result.then(() => { + settled = true + }) + expect(settled).toBe(false) + await expect(result).resolves.toBe(42) + expect(add).not.toHaveBeenCalled() }) test('observes a rejected value when the signal is already aborted', async () => { diff --git a/packages/router-core/tests/loader-architecture-regressions.test.ts b/packages/router-core/tests/loader-architecture-regressions.test.ts index 97561a9f727..7857a8bc7f6 100644 --- a/packages/router-core/tests/loader-architecture-regressions.test.ts +++ b/packages/router-core/tests/loader-architecture-regressions.test.ts @@ -8,6 +8,8 @@ import { redirect, } from '../src' import { createTestRouter, loadServerResponse } from './routerTestUtils' +import type { Readable } from '@tanstack/store' +import type { ParsedLocation } from '../src' test.each([ ['error', () => new Error('parent failed')], @@ -172,6 +174,62 @@ test('superseding a load clears fetching state from the still-presented lane', a await Promise.all([firstNavigation, secondNavigation]) }) +test('a store subscriber that navigates while the start publication flushes hands the lane over cleanly', async () => { + const pageGate = createControlledPromise() + const rootRoute = new BaseRootRoute({}) + const indexRoute = new BaseRoute({ + getParentRoute: () => rootRoute, + path: '/', + }) + const pageRoute = new BaseRoute({ + getParentRoute: () => rootRoute, + path: '/page', + beforeLoad: () => pageGate, + }) + const otherRoute = new BaseRoute({ + getParentRoute: () => rootRoute, + path: '/other', + loader: () => 'other data', + }) + const router = createTestRouter({ + routeTree: rootRoute.addChildren([indexRoute, pageRoute, otherRoute]), + history: createMemoryHistory({ initialEntries: ['/'] }), + }) + await router.load() + + // Publications are batched, so the subscriber runs at the flush and + // re-enters navigation before the first lane has done any work. + let redirected: Promise | undefined + // The test store factory creates reactive atoms. + const location = router.stores.location as unknown as Readable + const subscription = location.subscribe((next) => { + if (next.pathname === '/page' && !redirected) { + redirected = router.navigate({ to: '/other' }) + } + }) + const first = router.navigate({ to: '/page' }) + await Promise.all([first, redirected]) + subscription.unsubscribe() + + expect(router.state.location.pathname).toBe('/other') + expect(router.state.resolvedLocation?.pathname).toBe('/other') + expect(router.state.status).toBe('idle') + expect(router.state.matches.map((match) => match.routeId)).toEqual([ + rootRoute.id, + otherRoute.id, + ]) + expect(router.state.matches.at(-1)).toMatchObject({ + loaderData: 'other data', + isFetching: false, + }) + + // The superseded lane cannot publish once its gate opens. + const presented = router.state.matches + pageGate.resolve() + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(router.state.matches).toBe(presented) +}) + test('an ignored preload flight is released after a navigation has planned its loaders', async () => { const preloadFailure = createControlledPromise() const childGate = createControlledPromise() diff --git a/packages/solid-router/tests/store-updates-during-navigation.test.tsx b/packages/solid-router/tests/store-updates-during-navigation.test.tsx index 3ef09b437f1..d73e7929b26 100644 --- a/packages/solid-router/tests/store-updates-during-navigation.test.tsx +++ b/packages/solid-router/tests/store-updates-during-navigation.test.tsx @@ -136,7 +136,8 @@ describe("Store doesn't update *too many* times during navigation", () => { // This number should be as small as possible to minimize the amount of work // that needs to be done during a navigation. // Any change that increases this number should be investigated. - expect(updates).toBe(6) + // The beforeLoad → loader fetching transition now publishes once. + expect(updates).toBe(5) }) test('redirection in preload', async () => { diff --git a/packages/vue-router/tests/store-updates-during-navigation.test.tsx b/packages/vue-router/tests/store-updates-during-navigation.test.tsx index 54d9c2edffe..9a94c043085 100644 --- a/packages/vue-router/tests/store-updates-during-navigation.test.tsx +++ b/packages/vue-router/tests/store-updates-during-navigation.test.tsx @@ -143,7 +143,8 @@ describe("Store doesn't update *too many* times during navigation", () => { // that needs to be done during a navigation. // Any change that increases this number should be investigated. // Note: Vue has different update counts than React/Solid due to different reactivity - expect(updates).toBe(6) + // The beforeLoad → loader fetching transition now publishes once. + expect(updates).toBe(5) }) test('redirection in preload', async () => {