refactor(router-core): split loadMatches into its own file #4969
Conversation
|
View your CI Pipeline Execution ↗ for commit 8497e35
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-plugin
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-plugin
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-server-functions-client
@tanstack/start-server-functions-fetcher
@tanstack/start-server-functions-server
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/router-core/src/load-matches.ts (1)
279-310: Prevent stale/duplicate pending timers in setupPendingTimeoutIf setupPendingTimeout is called more than once for the same match, the current guard will skip creating a new timer and leave any previously scheduled timer intact. This can lead to suboptimal UX if conditions change and we need to re-arm the pending timer. Clearing any existing timeout before assigning a new one ensures only a single, up-to-date timer per match.
Apply this diff:
const match = inner.router.getMatch(matchId)! -if (match._nonReactive.pendingTimeout !== undefined) return @@ - if (shouldPending) { - const pendingTimeout = setTimeout(() => { - // Update the match and prematurely resolve the loadMatches promise so that - // the pending component can start rendering - triggerOnReady(inner) - }, pendingMs) - match._nonReactive.pendingTimeout = pendingTimeout - } + if (shouldPending) { + // Clear any existing timeout before setting a new one + if (match._nonReactive.pendingTimeout !== undefined) { + clearTimeout(match._nonReactive.pendingTimeout) + } + match._nonReactive.pendingTimeout = setTimeout(() => { + // Update the match and prematurely resolve the loadMatches promise so that + // the pending component can start rendering + triggerOnReady(inner) + }, pendingMs) + }
🧹 Nitpick comments (2)
packages/router-core/src/load-matches.ts (2)
584-606: Guard against negative parent index when setting parentMatchPromiseWhen index is 0, inner.matchPromises[index - 1] is undefined by intent. Make that explicit to avoid accidental negative indexing if this code is ever adapted.
- const parentMatchPromise = inner.matchPromises[index - 1] as any + const parentMatchPromise = + index > 0 ? inner.matchPromises[index - 1] : undefined
874-877: Avoid mutating the caller’s arg object in loadMatchesObject.assign(arg, …) mutates the incoming arg object. Prefer creating a fresh inner object for clarity and to avoid side effects.
- const inner: InnerLoadContext = Object.assign(arg, { - matchPromises: [], - }) + const inner: InnerLoadContext = { + ...arg, + matchPromises: [], + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/router-core/src/index.ts(1 hunks)packages/router-core/src/load-matches.ts(1 hunks)packages/router-core/src/router.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/router-core/src/load-matches.ts (4)
packages/router-core/src/index.ts (15)
AnyRouter(213-213)ParsedLocation(72-72)UpdateMatchFn(239-239)AnyRouteMatch(90-90)NotFoundError(378-378)isRedirect(373-373)isNotFound(379-379)AnyRoute(163-163)rootRouteId(111-111)createControlledPromise(279-279)BeforeLoadContextOptions(181-181)ControlledPromise(302-302)LoaderFnContext(176-176)MakeRouteMatch(89-89)componentTypes(42-42)packages/router-core/src/router.ts (2)
AnyRouter(692-692)UpdateMatchFn(659-662)packages/router-core/src/route.ts (1)
SsrContextOptions(945-964)packages/router-core/src/utils.ts (1)
isPromise(477-485)
packages/router-core/src/router.ts (2)
packages/router-core/src/load-matches.ts (2)
loadMatches(865-915)loadRouteChunk(917-956)packages/router-core/src/Matches.ts (1)
AnyRouteMatch(239-239)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (9)
packages/router-core/src/load-matches.ts (2)
917-956: loadRouteChunk return value can be undefined; ensure callers are OK with thatThis function may return undefined (no components to preload), which is fine at runtime, but note that RouterCore’s LoadRouteChunkFn is typed as Promise<Array>. If you rely on that type elsewhere, it may be misleading.
- Option A (typing only): Relax LoadRouteChunkFn to Promise<void | Array>.
- Option B (behavioral): Always return a promise (e.g., route._componentsPromise ?? Promise.resolve()).
If you want to go with Option B, minimally adjust the final return:
- return route._componentsPromise + return route._componentsPromise ?? Promise.resolve()
101-145: Redirect propagation looks correct; keep in mind error mutationSetting err.options._fromLocation and redirectHandled before resolveRedirect ensures proper context and idempotency. The mutation of the err variable inside this block is intentional and safe given the isRedirect guard.
packages/router-core/src/index.ts (2)
42-43: Public re-export of componentTypes moved to load-matchesThis aligns the public surface with the new module boundaries. Looks good.
42-43: No lingeringcomponentTypesimports foundI ran the provided
rgsearch across the repo and confirmed there are no remaining import/export statements pullingcomponentTypesfrom any path containing “router.” All references now resolve to./load-matches, so no compatibility re-export or additional updates are needed.packages/router-core/src/router.ts (5)
37-37: Delegation to load-matches module is clean and avoids runtime cyclesImporting loadMatches, loadRouteChunk, and routeNeedsPreload centralizes the loading lifecycle outside the RouterCore. Since load-matches only imports types from router, there’s no runtime circular dependency.
1890-1896: Passing updateMatch by reference is safe (arrow property preserves this binding)RouterCore.updateMatch is defined as an arrow function property, so its this binding is preserved when passing it down. The onReady workflow is also properly awaited later.
664-665: LoadRouteChunkFn return type may be too specificLoadRouteChunkFn is currently typed as Promise<Array>, but loadRouteChunk can resolve to undefined. Consider relaxing the type to Promise<void | Array> or Promise to reflect actual behavior and avoid confusing type expectations.
If you choose to relax the type, apply:
-export type LoadRouteChunkFn = (route: AnyRoute) => Promise<Array<void>> +export type LoadRouteChunkFn = (route: AnyRoute) => Promise<void | Array<void>>
2218-2231: Preload updateMatch override preserves active match consistencyThe conditional updater prevents overwriting active matches during preload, routing updates for inactive matches through the store. This matches the intended SWR semantics for preloading.
1248-1265: Using routeNeedsPreload at match creation preserves previous behaviorInitializing status to 'pending' when any loader/beforeLoad/lazy/preloadable component exists mirrors the old logic, now sourced from the external helper. Good separation.
Sheraff
deleted the
refactor-router-core-split-load-matches-to-new-file
branch
After the "flattening" PR #4961, we now have a very clear separate responsibility the
Routerclass is handling: loading matches.This PR proposes we split the code that handles "loading matches" into its own
load-matches.tsfile, with the entry points beingloadMatchesloadRouteChunkIn future PRs, we might consider splitting other responsibilities out of the 3k LoC
router.tsfile