-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 all 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. |
| 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,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) | ||
| } | ||
| }, | ||
| ) |
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