diff --git a/.changeset/cool-streets-punch.md b/.changeset/cool-streets-punch.md new file mode 100644 index 00000000000..2f813f30e22 --- /dev/null +++ b/.changeset/cool-streets-punch.md @@ -0,0 +1,5 @@ +--- +'@tanstack/router-core': patch +--- + +Reduce Promise allocations during client navigation and static server SSR policy resolution. Skip cancellable waits for synchronous beforeLoad results while preserving navigation cancellation. diff --git a/packages/router-core/INTERNALS.md b/packages/router-core/INTERNALS.md index d39c229768f..3314b5d2c3b 100644 --- a/packages/router-core/INTERNALS.md +++ b/packages/router-core/INTERNALS.md @@ -339,6 +339,11 @@ uses the active preload entry as its additional authority. `beforeLoad` context is not a cache. +Client `beforeLoad` only installs a cancellable wait for Promise results. +Synchronous context still crosses an `await` before the cancellation check: +a hook can queue a replacement navigation before its loader is planned. +Promise detection assumes ordinary Promise behavior. + A completed client preload never stores reusable `beforeLoad` output. When its loader data enters the route cache, the merged context is discarded; the same-ID route-local `_ctx` may remain reusable. A later navigation rebuilds the diff --git a/packages/router-core/src/load-client.ts b/packages/router-core/src/load-client.ts index fc966253e77..0d07ab18c5f 100644 --- a/packages/router-core/src/load-client.ts +++ b/packages/router-core/src/load-client.ts @@ -248,14 +248,14 @@ type CoordinatorRouter = AnyRouter & { type LoaderTask = [ index: number, outcome: Promise, - chunkFailure: Promise, + chunkFailure: Promise, candidate?: WorkMatch, ] type BackgroundLoaderTask = [ index: number, outcome: Promise, - chunkFailure: Promise, + chunkFailure: Promise, candidate: WorkMatch, ] @@ -429,15 +429,17 @@ async function contextualize( } try { setFetching(router, match, 'beforeLoad', options[0 /* controller */]) - const result = await waitFor( - beforeLoad({ - ...common, - search: match.search, - context: match.context, - ...router.options.additionalContext, - }), - signal, - ) + 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] } @@ -894,38 +896,41 @@ function createLoaderTask( reloadFailure ?? [SUCCESS, match.loaderData], ) - // The async wrapper catches synchronous preload failures without deferring work. - const chunkOutcome = (async (): Promise => { - const chunk = loadRouteChunk(route, undefined, onLazyReady) - if (chunk) { - await waitFor(chunk, options[0 /* controller */].signal) - } - })().catch((cause): IndexedOutcome | undefined => - lane[1 /* matches */].some( - (candidate, candidateIndex) => - candidateIndex <= index && - (candidate.status === 'error' || - candidate.status === 'notFound' || - candidate._notFound), - ) - ? undefined - : [index, normalizeLaneError(router, lane, route, cause, options)], - ) - const chunkFailure = chunkOutcome.then((failure) => - outcome.then((result) => { + // Keep thrown preloads and rejected chunks in the same task promise. + const chunkFailure = (async (): Promise => { + try { + const chunk = loadRouteChunk(route, undefined, onLazyReady) + if (chunk) { + await waitFor(chunk, options[0 /* controller */].signal) + } + } catch (cause) { if ( - blocking && - !failure && - result[0 /* kind */] === SUCCESS && - match.status === 'pending' && - !options[0 /* controller */].signal.aborted + !lane[1 /* matches */].some( + (candidate, candidateIndex) => + candidateIndex <= index && + (candidate.status === 'error' || + candidate.status === 'notFound' || + candidate._notFound), + ) ) { - match.status = 'success' - onReady?.() + return [ + index, + normalizeLaneError(router, lane, route, cause, options), + ] satisfies IndexedOutcome } - return failure - }), - ) + } + // Readiness requires both the component chunk and loader data. + const result = await outcome + if ( + blocking && + result[0 /* kind */] === SUCCESS && + match.status === 'pending' && + !options[0 /* controller */].signal.aborted + ) { + match.status = 'success' + onReady?.() + } + })() tasks.push([index, outcome, chunkFailure]) if (!background) { return outcome.then((result) => getParentSnapshot(match, result)) @@ -1982,7 +1987,7 @@ export async function loadClientRoute( ) const done = opts?.sync ? new Promise((resolve) => (settle = resolve)) - : Promise.resolve().then(run).then() + : Promise.resolve().then(run) const tx: LoadTransaction = [ controller, redirects, diff --git a/packages/router-core/src/load-server.ts b/packages/router-core/src/load-server.ts index f04011d1a4a..1991828d4fd 100644 --- a/packages/router-core/src/load-server.ts +++ b/packages/router-core/src/load-server.ts @@ -155,11 +155,11 @@ function waitFor(value: Promise, signal?: AbortSignal): Promise { return signal ? waitForReason(value, signal) : value } -async function resolveSsr( +function resolveSsr( router: AnyRouter, lane: MatchedLane, index: number, -): Promise { +): SSROption | Promise { const match = lane.matches[index]! const route = getRoute(router, match) const parentSsr = lane.matches[index - 1]?.ssr @@ -203,7 +203,14 @@ async function resolveSsr( ssr: candidate.ssr, })), } - return inherit((await option(context)) ?? defaultSsr) + try { + return Promise.resolve(option(context)).then((value) => + inherit(value ?? defaultSsr), + ) + } catch (cause) { + // Functional failures keep their asynchronous cancellation checkpoint. + return Promise.reject(cause) + } } function stampNotFound( @@ -232,7 +239,9 @@ async function contextualize( const match = lane.matches[index]! const route = getRoute(router, match) try { - match.ssr = await resolveSsr(router, lane, index) + const ssr = resolveSsr(router, lane, index) + // Functional policies are assimilated into a native Promise above. + match.ssr = ssr instanceof Promise ? await ssr : ssr } catch (cause) { signal?.throwIfAborted() failure = [ diff --git a/packages/router-core/tests/navigation-awaitable.test.ts b/packages/router-core/tests/navigation-awaitable.test.ts new file mode 100644 index 00000000000..1a0f165f93d --- /dev/null +++ b/packages/router-core/tests/navigation-awaitable.test.ts @@ -0,0 +1,200 @@ +import { runInNewContext } from 'node:vm' +import { describe, expect, test, vi } from 'vitest' +import { createMemoryHistory } from '@tanstack/history' +import { BaseRootRoute, BaseRoute, notFound, redirect } from '../src' +import { createTestRouter, loadServerResponse } from './routerTestUtils' + +describe.each([false, true])('awaitable hooks (server=%s)', (isServer) => { + test.each(['sync', 'promise', 'foreign promise'])( + 'inherits the result of a %s beforeLoad', + async (mode) => { + const value = { token: 'parent context' } + const root = new BaseRootRoute({ + beforeLoad: () => { + switch (mode) { + case 'promise': + return Promise.resolve(value) + case 'foreign promise': + return runInNewContext('Promise.resolve(value)', { value }) + default: + return value + } + }, + }) + const loader = vi.fn(({ context }) => context.token) + const child = new BaseRoute({ + getParentRoute: () => root, + path: '/', + loader, + }) + const router = createTestRouter({ + routeTree: root.addChildren([child]), + history: createMemoryHistory({ initialEntries: ['/'] }), + isServer, + }) + if (isServer) { + expect((await loadServerResponse(router, '/')).status).toBe(200) + } else { + await router.load() + } + expect(loader).toHaveBeenCalledOnce() + expect(router.state.matches.at(-1)?.loaderData).toBe(value.token) + }, + ) +}) + +test.each(['immediate', 'microtask'] as const)( + 'a %s replacement from beforeLoad does not start its stale loader', + async (mode) => { + const root = new BaseRootRoute({}) + const loader = vi.fn() + const stale = new BaseRoute({ + getParentRoute: () => root, + path: '/stale', + beforeLoad: ({ navigate }) => { + const replace = () => { + void navigate({ to: '/current' }) + } + if (mode === 'microtask') { + queueMicrotask(replace) + } else { + replace() + } + return { stale: true } + }, + loader, + }) + const current = new BaseRoute({ + getParentRoute: () => root, + path: '/current', + }) + const router = createTestRouter({ + routeTree: root.addChildren([stale, current]), + history: createMemoryHistory({ initialEntries: ['/stale'] }), + }) + await router.load() + expect(router.state.location.pathname).toBe('/current') + expect(loader).not.toHaveBeenCalled() + }, +) + +test.each(['native', 'foreign'] as const)( + 'supersedes an unresolved %s Promise beforeLoad and observes its late rejection', + async (mode) => { + let rejectValue!: (error: Error) => void + const capture = (_resolve: unknown, reject: typeof rejectValue) => { + rejectValue = reject + } + const pending = + mode === 'native' + ? new Promise(capture) + : runInNewContext('new Promise(capture)', { capture }) + const beforeLoad = vi.fn(() => pending) + const loader = vi.fn() + const onError = vi.fn() + const root = new BaseRootRoute({}) + const stale = new BaseRoute({ + getParentRoute: () => root, + path: '/stale', + beforeLoad, + loader, + onError, + }) + const current = new BaseRoute({ + getParentRoute: () => root, + path: '/current', + }) + const router = createTestRouter({ + routeTree: root.addChildren([stale, current]), + history: createMemoryHistory({ initialEntries: ['/stale'] }), + }) + const staleLoad = router.load() + await vi.waitFor(() => expect(beforeLoad).toHaveBeenCalledOnce()) + await router.navigate({ to: '/current' }) + await staleLoad + rejectValue(new Error('late failure')) + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(router.state.location.pathname).toBe('/current') + expect(loader).not.toHaveBeenCalled() + expect(onError).not.toHaveBeenCalled() + }, +) + +test.each(['throw', 'reject'] as const)( + 'a normal component preload can %s a redirect', + async (mode) => { + const root = new BaseRootRoute({}) + const from = new BaseRoute({ + getParentRoute: () => root, + path: '/from', + component: Object.assign(() => null, { + preload: () => { + const result = redirect({ to: '/to' }) + if (mode === 'throw') { + throw result + } + return Promise.reject(result) + }, + }) as any, + }) + const to = new BaseRoute({ getParentRoute: () => root, path: '/to' }) + const router = createTestRouter({ + routeTree: root.addChildren([from, to]), + history: createMemoryHistory({ initialEntries: ['/from'] }), + }) + await router.load() + expect(router.state.location.pathname).toBe('/to') + expect(router.state.matches.at(-1)?.status).toBe('success') + }, +) + +test.each(['throw', 'reject'] as const)( + 'a chunk %s supports reentrant onError control flow', + async (mode) => { + for (const control of ['navigate', 'redirect', 'notFound'] as const) { + const error = new Error('chunk failed') + const root = new BaseRootRoute({}) + const onError = vi.fn(() => { + if (control === 'navigate') { + void router.navigate({ to: '/current' }) + } else if (control === 'redirect') { + throw redirect({ to: '/current' }) + } else { + throw notFound() + } + }) + const stale = new BaseRoute({ + getParentRoute: () => root, + path: '/stale', + component: Object.assign(() => null, { + preload: () => { + if (mode === 'throw') { + throw error + } + return Promise.reject(error) + }, + }) as any, + notFoundComponent: (() => null) as any, + loader: control === 'navigate' ? () => 'obsolete data' : undefined, + onError, + }) + const current = new BaseRoute({ + getParentRoute: () => root, + path: '/current', + }) + const router = createTestRouter({ + routeTree: root.addChildren([stale, current]), + history: createMemoryHistory({ initialEntries: ['/stale'] }), + }) + await router.load() + expect(onError).toHaveBeenCalledExactlyOnceWith(error) + if (control === 'notFound') { + expect(router.state.matches.at(-1)?.status).toBe('notFound') + } else { + expect(router.state.location.pathname).toBe('/current') + expect(router.state.matches.at(-1)?.status).toBe('success') + } + expect(router._flights?.size ?? 0).toBe(0) + } + }, +) diff --git a/packages/router-core/tests/server-static-ssr.test.ts b/packages/router-core/tests/server-static-ssr.test.ts new file mode 100644 index 00000000000..21c1b8775a9 --- /dev/null +++ b/packages/router-core/tests/server-static-ssr.test.ts @@ -0,0 +1,97 @@ +import { runInNewContext } from 'node:vm' +import { expect, test, vi } from 'vitest' +import { createMemoryHistory } from '@tanstack/history' +import { BaseRootRoute, BaseRoute } from '../src' +import { createTestRouter, loadServerResponse } from './routerTestUtils' + +test.each(['false', 'data-only', 'default'] as const)( + 'inherits %s SSR through static and functional children', + async (policy) => { + for (const mode of [ + 'undefined', + 'true', + 'sync', + 'promise', + 'foreign', + 'thenable', + ]) { + const loader = vi.fn() + const root = new BaseRootRoute({ + ssr: + policy === 'default' + ? undefined + : policy === 'false' + ? false + : 'data-only', + }) + const child = new BaseRoute({ + getParentRoute: () => root, + path: '/', + ssr: + mode === 'undefined' + ? undefined + : mode === 'true' + ? true + : () => { + if (mode === 'foreign') { + return runInNewContext('Promise.resolve(true)') + } + if (mode === 'thenable') { + return { then: (resolve: any) => resolve(true) } as any + } + return mode === 'sync' ? true : Promise.resolve(true) + }, + loader, + }) + const router = createTestRouter({ + routeTree: root.addChildren([child]), + history: createMemoryHistory({ initialEntries: ['/'] }), + isServer: true, + }) + router.options.defaultSsr = policy === 'default' ? 'data-only' : true + expect((await loadServerResponse(router, '/')).status).toBe(200) + expect(router.state.matches.map((match) => match.ssr)).toEqual( + policy === 'false' ? [false, false] : ['data-only', 'data-only'], + ) + expect(loader).toHaveBeenCalledTimes(policy === 'false' ? 0 : 1) + } + }, +) + +test.each(['return', 'throw', 'microtask throw'] as const)( + 'request cancellation wins when an SSR callback aborts then %ss', + async (mode) => { + const controller = new AbortController() + const cancellation = new Error('disconnected') + const context = vi.fn() + const loader = vi.fn() + const onError = vi.fn() + const root = new BaseRootRoute({ + ssr: () => { + if (mode === 'microtask throw') { + queueMicrotask(() => controller.abort(cancellation)) + } else { + controller.abort(cancellation) + } + if (mode !== 'return') { + throw new Error('obsolete policy error') + } + return true + }, + context, + loader, + onError, + }) + const router = createTestRouter({ + routeTree: root, + history: createMemoryHistory({ initialEntries: ['/'] }), + isServer: true, + }) + await expect( + loadServerResponse(router, '/', controller.signal), + ).rejects.toBe(cancellation) + expect(context).not.toHaveBeenCalled() + expect(loader).not.toHaveBeenCalled() + expect(onError).not.toHaveBeenCalled() + }, +)