Repository navigation
perf(router-core): reduce navigation promise chains #8259
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
Changes from 5 commits
4d58964
d65e90e
220dbd9
c069c4b
b8e8fd5
46681ae
bbd9d9a
f05bd73
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/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. |
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -155,11 +155,11 @@ function waitFor<T>(value: Promise<T>, signal?: AbortSignal): Promise<T> { | |
| return signal ? waitForReason(value, signal) : value | ||
| } | ||
|
|
||
| async function resolveSsr( | ||
| function resolveSsr( | ||
| router: AnyRouter, | ||
| lane: MatchedLane, | ||
| index: number, | ||
| ): Promise<SSROption> { | ||
| ): SSROption | Promise<SSROption> { | ||
| 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 | ||
|
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 use
Collaborator
Author
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. if we know it's going to be a genuine |
||
| } catch (cause) { | ||
| signal?.throwIfAborted() | ||
| failure = [ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| import { createHook } from 'node:async_hooks' | ||
| import { bench, describe, expect } from 'vitest' | ||
| import { createMemoryHistory } from '@tanstack/history' | ||
| import { BaseRootRoute, BaseRoute } from '../src' | ||
| import { createTestRouter } from './routerTestUtils' | ||
| import type { AnyRoute } from '../src' | ||
|
|
||
| // Use the same cases on both implementations. Allocation counts are collected | ||
| // outside the timed loop so async-hooks instrumentation cannot bias timings. | ||
| for (const mode of [ | ||
| 'none', | ||
| 'sync', | ||
| 'void', | ||
| 'async', | ||
| 'mixed', | ||
| 'chunks', | ||
| 'blocking', | ||
| ] as const) { | ||
| const root = new BaseRootRoute({}) | ||
| let chunkCalls = 0 | ||
| let parent: AnyRoute = root | ||
| for (let index = 0; index < 8; index++) { | ||
| const parentRoute = parent | ||
| const value = { [`level${index}`]: index } | ||
| const child = new BaseRoute({ | ||
| getParentRoute: () => parentRoute, | ||
| path: `level${index}`, | ||
| beforeLoad: | ||
| mode === 'none' || mode === 'chunks' || mode === 'blocking' | ||
| ? undefined | ||
| : () => | ||
| mode === 'async' || (mode === 'mixed' && index % 4 === 0) | ||
| ? Promise.resolve(value) | ||
| : mode === 'void' | ||
| ? undefined | ||
| : value, | ||
| component: | ||
| mode === 'chunks' || mode === 'blocking' | ||
| ? (Object.assign(() => null, { | ||
| preload: () => { | ||
| chunkCalls++ | ||
| return Promise.resolve() | ||
| }, | ||
| }) as any) | ||
| : undefined, | ||
| shouldReload: mode === 'blocking' ? true : undefined, | ||
| loader: | ||
| mode === 'blocking' | ||
| ? { | ||
| handler: () => Promise.resolve(value), | ||
| staleReloadMode: 'blocking', | ||
| } | ||
| : undefined, | ||
| }) | ||
| parent.addChildren([child]) | ||
| parent = child | ||
| } | ||
| const router = createTestRouter({ | ||
| routeTree: root, | ||
| history: createMemoryHistory({ | ||
| initialEntries: [ | ||
| '/level0/level1/level2/level3/level4/level5/level6/level7', | ||
| ], | ||
| }), | ||
| }) | ||
| await router.load() | ||
| const navigate = () => router.navigate({ to: '.', replace: true }) | ||
| await navigate() | ||
| expect(router.state.matches).toHaveLength(9) | ||
| expect( | ||
| router.state.matches.every((match) => match.status === 'success'), | ||
| ).toBe(true) | ||
| if (mode === 'chunks' || mode === 'blocking') { | ||
| expect(chunkCalls).toBe(16) | ||
| if (mode === 'blocking') { | ||
| expect(router.state.matches[8]!.loaderData).toEqual({ level7: 7 }) | ||
| } | ||
| } else if (mode !== 'none' && mode !== 'void') { | ||
| expect(router.state.matches[8]!.context).toMatchObject({ | ||
| level0: 0, | ||
| level7: 7, | ||
| }) | ||
| } | ||
|
|
||
| let promises = 0 | ||
| const hook = createHook({ | ||
| init(_id, type) { | ||
| if (type === 'PROMISE') { | ||
| promises++ | ||
| } | ||
| }, | ||
| }) | ||
| hook.enable() | ||
| await navigate() | ||
| hook.disable() | ||
| console.info(`${mode}: ${promises} Promises per navigation`) | ||
|
|
||
| describe(`${mode} beforeLoad`, () => { | ||
| bench( | ||
| '10 navigations through 8 routes', | ||
| async () => { | ||
| for (let index = 0; index < 10; index++) { | ||
| await navigate() | ||
| } | ||
| }, | ||
| { time: 1500, warmupTime: 300 }, | ||
| ) | ||
| }) | ||
| } |
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.
π― Functional Correctness | π‘ Minor | β‘ Quick win
π Supported by static analysis
π Script executed:
Repository: TanStack/router
Length of output: 173
π€ get_repo_knowledge executed:
get_repo_knowledge TanStack/router /tmp/coderabbit-repo-knowledge/tanstack-router-7628dab7/architectureLength of output: 28039
π Script executed:
Repository: TanStack/router
Length of output: 8636
Read
thenonly once forbeforeLoadthenables.Line 439 reads
value.thento choose the asynchronous path.waitForthen callsPromise.resolve(value), which readsthenagain. A getter-backed thenable can throw or return a different method on the second read. Capturethenonce and use it for cancellation-aware assimilation. Add a regression test.π€ Prompt for AI Agents