Skip to content

refactor(router-core): fewer getMatch calls #4971

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Aug 16, 2025
Merged

refactor(router-core): fewer getMatch calls #4971

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1720f28
refactor(router-core): fewer getMatch calls
Sheraff Aug 16, 2025
179830b
keep onReady promise
Sheraff Aug 16, 2025
0ec2d48
ci: apply automated fixes
autofix-ci[bot] Aug 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
214 changes: 102 additions & 112 deletions packages/router-core/src/load-matches.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { isNotFound } from './not-found'
import { rootRouteId } from './root'
import { isRedirect } from './redirect'
import type { NotFoundError } from './not-found'
import type { ControlledPromise } from './utils'
import type { ParsedLocation } from './location'
import type {
AnyRoute,
Expand Down Expand Up @@ -56,12 +55,6 @@ const _handleNotFound = (inner: InnerLoadContext, err: NotFoundError) => {
// First check if a specific route is requested to show the error
const routeCursor =
inner.router.routesById[err.routeId ?? ''] ?? inner.router.routeTree
const matchesByRouteId: Record<string, AnyRouteMatch> = {}

// Setup routesByRouteId object for quick access
for (const match of inner.matches) {
matchesByRouteId[match.routeId] = match
}

// Ensure a NotFoundComponent exists on the route
if (
Expand All @@ -80,7 +73,7 @@ const _handleNotFound = (inner: InnerLoadContext, err: NotFoundError) => {
)

// Find the match for this route
const matchForRoute = matchesByRouteId[routeCursor.id]
const matchForRoute = inner.matches.find((m) => m.routeId === routeCursor.id)

invariant(matchForRoute, 'Could not find match for route: ' + routeCursor.id)

Expand Down Expand Up @@ -246,7 +239,7 @@ const isBeforeLoadSsr = (
existingMatch.ssr = parentOverride(route.options.ssr)
return
}
const { search, params } = inner.router.getMatch(matchId)!
const { search, params } = existingMatch

const ssrFnContext: SsrContextOptions<any, any, any> = {
search: makeMaybe(search, existingMatch.searchError),
Expand Down Expand Up @@ -280,8 +273,8 @@ const setupPendingTimeout = (
inner: InnerLoadContext,
matchId: string,
route: AnyRoute,
match: AnyRouteMatch,
): void => {
const match = inner.router.getMatch(matchId)!
if (match._nonReactive.pendingTimeout !== undefined) return

const pendingMs =
Expand Down Expand Up @@ -324,20 +317,20 @@ const shouldExecuteBeforeLoad = (
)
return true

setupPendingTimeout(inner, matchId, route)
setupPendingTimeout(inner, matchId, route, existingMatch)

const then = () => {
let shouldExecuteBeforeLoad = true
let result = true
const match = inner.router.getMatch(matchId)!
if (match.status === 'error') {
shouldExecuteBeforeLoad = true
result = true
} else if (
match.preload &&
(match.status === 'redirected' || match.status === 'notFound')
) {
handleRedirectAndNotFound(inner, match, match.error)
}
return shouldExecuteBeforeLoad
return result
}
Comment on lines 322 to 334
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the result is always true here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn you're right. The PR that made this change is this one though: #4961. I'll try and see if I can track down the original logic, but if you know what it's supposed to be already, I'm all ears!

Copy link
Contributor Author

@Sheraff Sheraff Aug 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it looks like it's been always true since this older PR https://github.com/TanStack/router/pull/4550/files (2 months ago). So I think we can at least say that it's not causing things to break. But maybe it is sub-optimal and we're executing too many beforeLoad calls. @schiller-manuel do you remember this a little?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a PR for the cleanup #4992, and a follow up (wip) for actually re-thinking what should happen there #4993


// Wait for the beforeLoad to resolve before we continue
Expand All @@ -354,7 +347,6 @@ const executeBeforeLoad = (
): void | Promise<void> => {
const match = inner.router.getMatch(matchId)!

match._nonReactive.beforeLoadPromise = createControlledPromise<void>()
// explicitly capture the previous loadPromise
const prevLoadPromise = match._nonReactive.loadPromise
match._nonReactive.loadPromise = createControlledPromise<void>(() => {
Expand All @@ -371,7 +363,7 @@ const executeBeforeLoad = (
handleSerialError(inner, index, searchError, 'VALIDATE_SEARCH')
}

setupPendingTimeout(inner, matchId, route)
setupPendingTimeout(inner, matchId, route, match)

const abortController = new AbortController()

Expand Down Expand Up @@ -415,6 +407,8 @@ const executeBeforeLoad = (
return
}

match._nonReactive.beforeLoadPromise = createControlledPromise<void>()

const { search, params, cause } = match
const preload = resolvePreload(inner, matchId)
const beforeLoadFnContext: BeforeLoadContextOptions<any, any, any, any, any> =
Expand Down Expand Up @@ -510,8 +504,8 @@ const handleBeforeLoad = (
: execute(shouldExecuteBeforeLoadResult)
}

const execute = (shouldExecuteBeforeLoad: boolean) => {
if (shouldExecuteBeforeLoad) {
const execute = (shouldExec: boolean) => {
if (shouldExec) {
// If we are not in the middle of a load OR the previous load failed, start it
return executeBeforeLoad(inner, matchId, index, route)
}
Expand Down Expand Up @@ -567,14 +561,6 @@ const executeHead = (
})
}

const potentialPendingMinPromise = (
inner: InnerLoadContext,
matchId: string,
): void | ControlledPromise<void> => {
const latestMatch = inner.router.getMatch(matchId)!
return latestMatch._nonReactive.minPendingPromise
}

const getLoaderContext = (
inner: InnerLoadContext,
matchId: string,
Expand All @@ -592,7 +578,7 @@ const getLoaderContext = (
deps: loaderDeps,
preload: !!preload,
parentMatchPromise,
abortController: abortController,
abortController,
context,
location: inner.location,
navigate: (opts) =>
Expand All @@ -618,12 +604,11 @@ const runLoader = async (
// before committing to the match and resolving
// the loadPromise

const match = inner.router.getMatch(matchId)!

// Actually run the loader and handle the result
try {
if (
!inner.router.isServer ||
inner.router.getMatch(matchId)!.ssr === true
) {
if (!inner.router.isServer || match.ssr === true) {
loadRouteChunk(route)
}

Expand All @@ -641,7 +626,7 @@ const runLoader = async (
route.options.head ||
route.options.scripts ||
route.options.headers ||
inner.router.getMatch(matchId)!._nonReactive.minPendingPromise
match._nonReactive.minPendingPromise
)

if (willLoadSomething) {
Expand Down Expand Up @@ -675,7 +660,7 @@ const runLoader = async (
if (route._lazyPromise) await route._lazyPromise
const headResult = executeHead(inner, matchId, route)
const head = headResult ? await headResult : undefined
const pendingPromise = potentialPendingMinPromise(inner, matchId)
const pendingPromise = match._nonReactive.minPendingPromise
if (pendingPromise) await pendingPromise

// Last but not least, wait for the the components
Expand All @@ -692,7 +677,7 @@ const runLoader = async (
} catch (e) {
let error = e

const pendingPromise = potentialPendingMinPromise(inner, matchId)
const pendingPromise = match._nonReactive.minPendingPromise
if (pendingPromise) await pendingPromise

handleRedirectAndNotFound(inner, inner.router.getMatch(matchId), e)
Expand Down Expand Up @@ -744,7 +729,6 @@ const loadRouteMatch = async (
let loaderIsRunningAsync = false
const route = inner.router.looseRoutesById[routeId]!

const prevMatch = inner.router.getMatch(matchId)!
if (shouldSkipLoader(inner, matchId)) {
if (inner.router.isServer) {
const headResult = executeHead(inner, matchId, route)
Expand All @@ -757,88 +741,92 @@ const loadRouteMatch = async (
}
return inner.router.getMatch(matchId)!
}
}
// there is a loaderPromise, so we are in the middle of a load
else if (prevMatch._nonReactive.loaderPromise) {
// do not block if we already have stale data we can show
// but only if the ongoing load is not a preload since error handling is different for preloads
// and we don't want to swallow errors
if (prevMatch.status === 'success' && !inner.sync && !prevMatch.preload) {
return inner.router.getMatch(matchId)!
}
await prevMatch._nonReactive.loaderPromise
const match = inner.router.getMatch(matchId)!
if (match.error) {
handleRedirectAndNotFound(inner, match, match.error)
}
} else {
// This is where all of the stale-while-revalidate magic happens
const age = Date.now() - inner.router.getMatch(matchId)!.updatedAt

const preload = resolvePreload(inner, matchId)

const staleAge = preload
? (route.options.preloadStaleTime ??
inner.router.options.defaultPreloadStaleTime ??
30_000) // 30 seconds for preloads by default
: (route.options.staleTime ?? inner.router.options.defaultStaleTime ?? 0)

const shouldReloadOption = route.options.shouldReload

// Default to reloading the route all the time
// Allow shouldReload to get the last say,
// if provided.
const shouldReload =
typeof shouldReloadOption === 'function'
? shouldReloadOption(getLoaderContext(inner, matchId, index, route))
: shouldReloadOption

const nextPreload =
!!preload && !inner.router.state.matches.some((d) => d.id === matchId)
const match = inner.router.getMatch(matchId)!
match._nonReactive.loaderPromise = createControlledPromise<void>()
if (nextPreload !== match.preload) {
inner.updateMatch(matchId, (prev) => ({
...prev,
preload: nextPreload,
}))
}

// If the route is successful and still fresh, just resolve
const { status, invalid } = inner.router.getMatch(matchId)!
loaderShouldRunAsync =
status === 'success' && (invalid || (shouldReload ?? age > staleAge))
if (preload && route.options.preload === false) {
// Do nothing
} else if (loaderShouldRunAsync && !inner.sync) {
loaderIsRunningAsync = true
;(async () => {
try {
await runLoader(inner, matchId, index, route)
const match = inner.router.getMatch(matchId)!
match._nonReactive.loaderPromise?.resolve()
match._nonReactive.loadPromise?.resolve()
match._nonReactive.loaderPromise = undefined
} catch (err) {
if (isRedirect(err)) {
await inner.router.navigate(err.options)
}
}
})()
} else if (status !== 'success' || (loaderShouldRunAsync && inner.sync)) {
await runLoader(inner, matchId, index, route)
const prevMatch = inner.router.getMatch(matchId)!
// there is a loaderPromise, so we are in the middle of a load
if (prevMatch._nonReactive.loaderPromise) {
// do not block if we already have stale data we can show
// but only if the ongoing load is not a preload since error handling is different for preloads
// and we don't want to swallow errors
if (prevMatch.status === 'success' && !inner.sync && !prevMatch.preload) {
return prevMatch
}
await prevMatch._nonReactive.loaderPromise
const match = inner.router.getMatch(matchId)!
if (match.error) {
handleRedirectAndNotFound(inner, match, match.error)
}
} else {
// if the loader did not run, still update head.
// reason: parent's beforeLoad may have changed the route context
// and only now do we know the route context (and that the loader would not run)
const headResult = executeHead(inner, matchId, route)
if (headResult) {
const head = await headResult
// This is where all of the stale-while-revalidate magic happens
const age = Date.now() - prevMatch.updatedAt

const preload = resolvePreload(inner, matchId)

const staleAge = preload
? (route.options.preloadStaleTime ??
inner.router.options.defaultPreloadStaleTime ??
30_000) // 30 seconds for preloads by default
: (route.options.staleTime ??
inner.router.options.defaultStaleTime ??
0)

const shouldReloadOption = route.options.shouldReload

// Default to reloading the route all the time
// Allow shouldReload to get the last say,
// if provided.
const shouldReload =
typeof shouldReloadOption === 'function'
? shouldReloadOption(getLoaderContext(inner, matchId, index, route))
: shouldReloadOption

const nextPreload =
!!preload && !inner.router.state.matches.some((d) => d.id === matchId)
const match = inner.router.getMatch(matchId)!
match._nonReactive.loaderPromise = createControlledPromise<void>()
if (nextPreload !== match.preload) {
inner.updateMatch(matchId, (prev) => ({
...prev,
...head,
preload: nextPreload,
}))
}

// If the route is successful and still fresh, just resolve
const { status, invalid } = match
loaderShouldRunAsync =
status === 'success' && (invalid || (shouldReload ?? age > staleAge))
if (preload && route.options.preload === false) {
// Do nothing
} else if (loaderShouldRunAsync && !inner.sync) {
loaderIsRunningAsync = true
;(async () => {
try {
await runLoader(inner, matchId, index, route)
const match = inner.router.getMatch(matchId)!
match._nonReactive.loaderPromise?.resolve()
match._nonReactive.loadPromise?.resolve()
match._nonReactive.loaderPromise = undefined
} catch (err) {
if (isRedirect(err)) {
await inner.router.navigate(err.options)
}
}
})()
} else if (status !== 'success' || (loaderShouldRunAsync && inner.sync)) {
await runLoader(inner, matchId, index, route)
} else {
// if the loader did not run, still update head.
// reason: parent's beforeLoad may have changed the route context
// and only now do we know the route context (and that the loader would not run)
const headResult = executeHead(inner, matchId, route)
if (headResult) {
const head = await headResult
inner.updateMatch(matchId, (prev) => ({
...prev,
...head,
}))
}
}
}
}
const match = inner.router.getMatch(matchId)!
Expand All @@ -858,8 +846,10 @@ const loadRouteMatch = async (
isFetching: nextIsFetching,
invalid: false,
}))
return inner.router.getMatch(matchId)!
} else {
return match
}
return inner.router.getMatch(matchId)!
}

export async function loadMatches(arg: {
Expand Down