Skip to content

refactor(router-core): flatten loadRouteMatch, can run synchronously #5018

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

refactor(router-core): flatten loadRouteMatch, can run synchronously #5018

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
85f379c
refactor(router-core): flatten loadRouteMatch, can run synchronously
Sheraff Aug 23, 2025
1a6d262
type-safe parentMatchPromise
Sheraff Aug 23, 2025
0c18913
ci: apply automated fixes
autofix-ci[bot] Aug 23, 2025
b070a68
fewer store updates in sync loadRouteMatch
Sheraff Aug 23, 2025
9ea4bc7
revert batching as i cannot measure the reduction
Sheraff Aug 23, 2025
2373200
ci: apply automated fixes
autofix-ci[bot] Aug 23, 2025
8b9e00a
revert public API change
Sheraff Aug 23, 2025
fead952
ci: apply automated fixes
autofix-ci[bot] Aug 23, 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
235 changes: 129 additions & 106 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 @@ -4,6 +4,7 @@ import { createControlledPromise, isPromise } from './utils'
import { isNotFound } from './not-found'
import { rootRouteId } from './root'
import { isRedirect } from './redirect'
import type { Awaitable } from './utils'
import type { NotFoundError } from './not-found'
import type { ParsedLocation } from './location'
import type {
Expand Down Expand Up @@ -34,7 +35,7 @@ type InnerLoadContext = {
onReady?: () => Promise<void>
sync?: boolean
/** mutable state, scoped to a `loadMatches` call */
matchPromises: Array<Promise<AnyRouteMatch>>
matchPromises: Array<Awaitable<AnyRouteMatch>>
}

const triggerOnReady = (inner: InnerLoadContext): void | Promise<void> => {
Expand Down Expand Up @@ -703,10 +704,10 @@ const runLoader = async (
}
}

const loadRouteMatch = async (
const loadRouteMatch = (
inner: InnerLoadContext,
index: number,
): Promise<AnyRouteMatch> => {
): Awaitable<AnyRouteMatch> => {
const { id: matchId, routeId } = inner.matches[index]!
let loaderShouldRunAsync = false
let loaderIsRunningAsync = false
Expand All @@ -716,121 +717,140 @@ const loadRouteMatch = async (
if (inner.router.isServer) {
const headResult = executeHead(inner, matchId, route)
if (headResult) {
const head = await headResult
inner.updateMatch(matchId, (prev) => ({
...prev,
...head,
}))
return headResult.then((head) => {
inner.updateMatch(matchId, (prev) => ({
...prev,
...head,
}))
return inner.router.getMatch(matchId)!
})
}
return inner.router.getMatch(matchId)!
}
} else {
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
return settleLoadRouteMatch()
}

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
}
return prevMatch._nonReactive.loaderPromise.then(() => {
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() - 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,
preload: nextPreload,
}))
}
return settleLoadRouteMatch()
})
}

// 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,
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)) {
if (preload && route.options.preload === false) {
// Do nothing
return settleLoadRouteMatch()
}

// If the route is successful and still fresh, just resolve
const { status, invalid } = match
loaderShouldRunAsync =
status === 'success' && (invalid || (shouldReload ?? age > staleAge))
if (loaderShouldRunAsync && !inner.sync) {
loaderIsRunningAsync = true
;(async () => {
try {
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)!
match._nonReactive.loaderPromise?.resolve()
match._nonReactive.loadPromise?.resolve()
match._nonReactive.loaderPromise = undefined
} catch (err) {
if (isRedirect(err)) {
await inner.router.navigate(err.options)
}
}
}
})()
return settleLoadRouteMatch()
}
Comment on lines +791 to 808
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid unhandled rejections during background redirects.

The async IIFE isn’t observed; awaiting inner.router.navigate(...) can leak unhandled rejections if navigation fails. Fire-and-forget instead.

-      } catch (err) {
-        if (isRedirect(err)) {
-          await inner.router.navigate(err.options)
-        }
-      }
+      } catch (err) {
+        if (isRedirect(err)) {
+          // Detach to avoid unhandled rejections in the background task
+          void inner.router.navigate(err.options)
+        }
+      }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
status === 'success' && (invalid || (shouldReload ?? age > staleAge))
if (loaderShouldRunAsync && !inner.sync) {
loaderIsRunningAsync = true
;(async () => {
try {
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)!
match._nonReactive.loaderPromise?.resolve()
match._nonReactive.loadPromise?.resolve()
match._nonReactive.loaderPromise = undefined
} catch (err) {
if (isRedirect(err)) {
await inner.router.navigate(err.options)
}
}
}
})()
return settleLoadRouteMatch()
}
status === 'success' && (invalid || (shouldReload ?? age > staleAge))
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)) {
// Detach to avoid unhandled rejections in the background task
void inner.router.navigate(err.options)
}
}
})()
return settleLoadRouteMatch()
}
🤖 Prompt for AI Agents 
In packages/router-core/src/load-matches.ts around lines 784 to 801, the async
IIFE currently does "await inner.router.navigate(err.options)" inside the catch
block which can leak unhandled rejections when navigation fails; instead call
navigate in a fire-and-forget manner and ensure any rejection is handled locally
(e.g. invoke inner.router.navigate(err.options).catch(() => {/* optional log
*/}) or drop the await and attach a .catch handler) so the background redirect
cannot create an unhandled rejection.

const match = inner.router.getMatch(matchId)!
if (!loaderIsRunningAsync) {
match._nonReactive.loaderPromise?.resolve()
match._nonReactive.loadPromise?.resolve()

if (status !== 'success' || (loaderShouldRunAsync && inner.sync)) {
return runLoader(inner, matchId, index, route).then(settleLoadRouteMatch)
}

clearTimeout(match._nonReactive.pendingTimeout)
match._nonReactive.pendingTimeout = undefined
if (!loaderIsRunningAsync) match._nonReactive.loaderPromise = undefined
match._nonReactive.dehydrated = undefined
const nextIsFetching = loaderIsRunningAsync ? match.isFetching : false
if (nextIsFetching !== match.isFetching || match.invalid !== false) {
inner.updateMatch(matchId, (prev) => ({
...prev,
isFetching: nextIsFetching,
invalid: false,
}))
return inner.router.getMatch(matchId)!
} 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) {
return headResult.then((head) => {
inner.updateMatch(matchId, (prev) => ({
...prev,
...head,
}))
return settleLoadRouteMatch()
})
}

return settleLoadRouteMatch()

function settleLoadRouteMatch() {
const match = inner.router.getMatch(matchId)!

if (!loaderIsRunningAsync) {
match._nonReactive.loaderPromise?.resolve()
match._nonReactive.loadPromise?.resolve()
match._nonReactive.loaderPromise = undefined
}

clearTimeout(match._nonReactive.pendingTimeout)
match._nonReactive.pendingTimeout = undefined
match._nonReactive.dehydrated = undefined

const nextIsFetching = loaderIsRunningAsync ? match.isFetching : false
if (nextIsFetching !== match.isFetching || match.invalid !== false) {
inner.updateMatch(matchId, (prev) => ({
...prev,
isFetching: nextIsFetching,
invalid: false,
}))
return inner.router.getMatch(matchId)!
}

return match
}
}
Expand Down Expand Up @@ -866,10 +886,13 @@ export async function loadMatches(arg: {

// Execute all loaders in parallel
const max = inner.firstBadMatchIndex ?? inner.matches.length
let hasPromises = false
for (let i = 0; i < max; i++) {
inner.matchPromises.push(loadRouteMatch(inner, i))
const result = loadRouteMatch(inner, i)
inner.matchPromises.push(result)
if (!hasPromises && isPromise(result)) hasPromises = true
}
await Promise.all(inner.matchPromises)
if (hasPromises) await Promise.all(inner.matchPromises)

const readyPromise = triggerOnReady(inner)
if (isPromise(readyPromise)) await readyPromise
Expand Down
2 changes: 1 addition & 1 deletion packages/router-core/src/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1236,7 +1236,7 @@ export interface LoaderFnContext<
// root route does not have a parent match
parentMatchPromise: TId extends RootRouteId
? never
: Promise<MakeRouteMatchFromRoute<TParentRoute>>
: Awaitable<MakeRouteMatchFromRoute<TParentRoute>>
cause: 'preload' | 'enter' | 'stay'
route: AnyRoute
}
Expand Down
Loading