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

Changes from all commits
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
239 changes: 131 additions & 108 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 @@ -550,7 +551,9 @@ const getLoaderContext = (
index: number,
route: AnyRoute,
): LoaderFnContext => {
const parentMatchPromise = inner.matchPromises[index - 1] as any
const parentMatchPromise = Promise.resolve(
inner.matchPromises[index - 1] as any,
)
Comment on lines +554 to +556
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Don’t wrap parentMatchPromise in Promise.resolve — preserve the new union type.

Wrapping with Promise.resolve(...) erases the benefit of the new public type (RouteMatch | Promise<RouteMatch>) by always forcing a microtask hop. Pass the underlying Awaitable through so a loader can synchronously access an already-settled parent when available.

Apply this diff:

-  const parentMatchPromise = Promise.resolve(
-    inner.matchPromises[index - 1] as any,
-  )
+  const parentMatchPromise =
+    inner.matchPromises[index - 1] as Awaitable<AnyRouteMatch>

Run this to confirm the public type actually advertises the union and that no downstream code assumes Promise<...>:

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the LoaderFnContext type to ensure the union is declared
rg -nC3 -P '(type|interface)\s+LoaderFnContext\b' packages | sed -n '1,200p' || true
rg -nC2 -P 'parentMatchPromise\s*:\s*.*\|' packages || true

# Detect call sites still assuming Promise<...>
rg -nC2 -P 'parentMatchPromise\s*:\s*Promise<' packages || true
rg -nC2 -P 'await\s+[^;\n]*parentMatchPromise\b' packages || true

Length of output: 8611

Preserve the Awaitable type for parentMatchPromise and update affected tests

To retain the new RouteMatch | Promise<RouteMatch> union and avoid unnecessary microtask hops, remove the Promise.resolve wrapper in load-matches.ts:

--- a/packages/router-core/src/load-matches.ts
+++ b/packages/router-core/src/load-matches.ts
@@ -554,7 +554,7 @@
-  const parentMatchPromise = Promise.resolve(
-    inner.matchPromises[index - 1] as any,
-  )
+  const parentMatchPromise =
+    inner.matchPromises[index - 1] as Awaitable<AnyRouteMatch>

Because this change surfaces a union instead of always a Promise, update the test declarations that currently assume Promise<…> for parentMatchPromise. For example:

• packages/solid-router/tests/route.test-d.tsx
– Lines 415, 461, 655, 680, 704, 807, 1166
• change
parentMatchPromise: Promise<MakeRouteMatchFromRoute<typeof rootRoute>>
to
parentMatchPromise: Promise<MakeRouteMatchFromRoute<typeof rootRoute>> | MakeRouteMatchFromRoute<typeof rootRoute>

• packages/solid-router/tests/loaders.test.tsx
– Line 138
• update the loader context signature to reflect Awaitable<…> for parentMatchPromise

• packages/react-router/tests/route.test-d.tsx
– Lines 411, 462, 684, 709, 733, 836, 1203
• apply the same union-type adjustment as above

• packages/react-router/tests/loaders.test.tsx
– Line 159
• ensure the loader context declaration uses Awaitable<…>

Making these test updates is required so that the public API’s union type is correctly represented in both runtime and type-level tests.

📝 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
const parentMatchPromise = Promise.resolve(
inner.matchPromises[index - 1] as any,
)
// Preserve the Awaitable<AnyRouteMatch> type instead of always wrapping in a Promise
const parentMatchPromise =
inner.matchPromises[index - 1] as Awaitable<AnyRouteMatch>
🤖 Prompt for AI Agents 
In packages/router-core/src/load-matches.ts around lines 554 to 556, remove the
Promise.resolve wrapper so parentMatchPromise preserves the Awaitable type
(i.e., allow RouteMatch | Promise<RouteMatch> rather than always wrapping in a
Promise), and then update the affected test declarations to reflect the
union/Awaitable type: change the listed parentMatchPromise types in
packages/solid-router/tests/route.test-d.tsx (lines 415, 461, 655, 680, 704,
807, 1166) and packages/react-router/tests/route.test-d.tsx (lines 411, 462,
684, 709, 733, 836, 1203) from Promise<…> to Promise<…> | …
(MakeRouteMatchFromRoute<typeof rootRoute>), and update the loader context
signatures in packages/solid-router/tests/loaders.test.tsx (line 138) and
packages/react-router/tests/loaders.test.tsx (line 159) to use Awaitable<…>;
ensure types compile after these changes.

const { params, loaderDeps, abortController, context, cause } =
inner.router.getMatch(matchId)!

Expand Down Expand Up @@ -703,134 +706,154 @@ 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
const route = inner.router.looseRoutesById[routeId]!
const prevMatch = inner.router.getMatch(matchId)!
let loaderIsRunningAsync = false

if (shouldSkipLoader(inner, matchId)) {
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)!
return prevMatch
}
} 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()
}

// 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)
prevMatch._nonReactive.loaderPromise = createControlledPromise<void>()
if (nextPreload !== prevMatch.preload) {
inner.updateMatch(matchId, (prev) => ({
...prev,
preload: nextPreload,
}))
}

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 (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 the route is successful and still fresh, just resolve
const { status, invalid } = prevMatch
const 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) => {
let result: ReturnType<typeof settleLoadRouteMatch>
batch(() => {
inner.updateMatch(matchId, (prev) => ({
...prev,
...head,
}))
result = settleLoadRouteMatch()
})
return result!
})
}

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