reafactor(router-core): remove cachedMatches and pendingMatches from store#6676
reafactor(router-core): remove cachedMatches and pendingMatches from store#6676
Conversation
…cached and pending matches
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR extracts Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Client (UI)
participant Core as RouterCore
participant Store as InternalStore (InternalStoreState)
participant Dev as Devtools
participant FW as Framework Hook (useMatch / useRouter)
UI->>Core: navigate(location)
Core->>Store: set pendingMatches / cachedMatches (pending)
Core->>FW: emit loader start / notify hooks
FW-->>Core: loader resolves/rejects
Core->>Store: commit -> move pending -> cached / clear pending
Store-->>Dev: subscription update (devtoolsMatches)
Dev->>UI: render Pending / Active / Cached UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit a5fcf0f
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/router/api/router/RouterStateType.md`:
- Line 6: Update the documentation sentence for the RouterState type to correct
grammar by adding "the": change "The `RouterState` type represents shape of the
internal state of the router." to "The `RouterState` type represents the shape
of the internal state of the router." Ensure this edit is applied in the
RouterStateType.md file where the `RouterState` symbol is described.
In `@packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx`:
- Around line 150-152: The computed matches currently uses
devtoolsMatches().pendingMatches || routerState().matches which treats an empty
array as truthy and causes an empty pendingMatches to shadow the real matches;
update the createMemo for matches to check for a non-empty pendingMatches (e.g.,
capture pending = devtoolsMatches().pendingMatches and return pending &&
pending.length > 0 ? pending : routerState().matches) so it falls back to
routerState().matches when pendingMatches is empty or missing.
🧹 Nitpick comments (3)
packages/vue-router/src/useMatch.tsx (1)
86-94: Consider memoizing thematchRoutescall.The
router.matchRoutes()call inside the selector runs synchronously on each state update when no match is found. Sincerouter.latestLocationmay not change between consecutive selector invocations, this computation could be redundant.This is likely acceptable given the early
state.status === 'pending'guard and the fact this only executes in the fallback path, but worth noting for future optimization if performance profiling reveals issues.packages/solid-router/src/useMatch.tsx (1)
89-101: Avoidanyin the pending-match predicate.This can stay fully typed by letting
matchRoutesinfer the match type.
As per coding guidelines:**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.♻️ Suggested change
- .some((d: any) => + .some((d) => opts.from ? opts.from === d.routeId : d.id === nearestMatchId(), )packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx (1)
310-315: Prefer typed matches instead ofanyin devtools views.These callbacks currently use
any, which weakens strict typing. You can type the memo and rely on inference in the iterators.♻️ Suggested tightening
- const displayedMatches = createMemo(() => - pendingMatches().length ? pendingMatches() : routerState().matches, - ) + const displayedMatches = createMemo<Array<AnyRouteMatch>>(() => + pendingMatches().length ? pendingMatches() : routerState().matches, + )- {displayedMatches().map((match: any, _i: any) => { + {displayedMatches().map((match) => {- {devtoolsMatches().cachedMatches.map((match: any) => { + {devtoolsMatches().cachedMatches.map((match) => {- {pendingMatches().find( - (d: any) => d.id === activeMatch()?.id, - ) + {pendingMatches().find( + (d) => d.id === activeMatch()?.id, + )- : routerState().matches.find( - (d: any) => d.id === activeMatch()?.id, - ) + : routerState().matches.find( + (d) => d.id === activeMatch()?.id, + )As per coding guidelines,
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.Also applies to: 564-597, 648-687, 719-725
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/vue-router/src/useMatch.tsx`:
- Around line 90-95: Remove the explicit any type in the pending-match predicate
so TypeScript can infer the correct match type from matchRoutes(); specifically
update the arrow callback passed to
.matchRoutes(router.latestLocation).some(...) (the predicate referencing
d.routeId and d.id and using nearestMatchId.value and opts.from) to omit the ":
any" annotation so d is strongly typed as the MakeRouteMatch union returned by
matchRoutes.
🧹 Nitpick comments (1)
packages/router-core/src/router.ts (1)
1133-1145: Minor inconsistency: initialpendingMatchesvalue vs type definition.The
InternalStoreStateinterface definespendingMatchesas optional (pendingMatches?: Array<AnyRouteMatch>), suggesting it can beundefined. However, the initial state sets it to an empty array[]. After navigation commits (Line 2451), it's set toundefined.This works correctly but is slightly inconsistent. Consider either:
- Initializing with
pendingMatches: undefinedto match the post-commit state, or- Keeping the type as
Array<AnyRouteMatch>(required) and using empty array consistentlyThe current implementation functions correctly since both
undefinedand[]are falsy for array length checks, but clarifying the semantic intent would improve maintainability.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/vue-router/src/useMatch.tsx`:
- Around line 87-94: The pending-match check in useMatch (the hasPendingMatch
logic reading router.internalStore.state.pendingMatches) can become stale
because internalStore updates may not trigger router.state selectors; update the
useMatch hook to also subscribe to or watch router.internalStore's
pendingMatches so changes cause a recompute — e.g., read pendingMatches from a
reactive source or call router.internalStore.subscribe and update local
state/ref used in the hasPendingMatch calculation, ensure you reference symbols
like hasPendingMatch, pendingError, router.internalStore.state.pendingMatches
and tear down the subscription on unmount so pendingError is cleared when
pendingMatches appears.
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
This PR moves
pendingMatchesandcachedMatchesout of publicrouter.stateinto a new internal store (router.internalStore) to reduce reactive store churn and hide internals. It then updates router-core/devtools/HMR/framework hooks/tests/docs to use the new internal location and documents migration away fromrouter.state.pendingMatches.Here's a graph showing the evolution of the number of store updates during multiple scenarios (see https://github.com/TanStack/router/blob/08a2d090be9b08af625bbf894a05a6375f436ecf/packages/react-router/tests/store-updates-during-navigation.test.tsx). On the x-axis,
#16representsmainwithout this PR's changes, and#18represents this PR's results.This is PR 4/4, the other 3 were just "working" PRs to get to this stage:
And it supersedes the previous attempts
Summary by CodeRabbit
Breaking Changes
New Features
Documentation
Tests