perf(solid-router): drop deep equality in selector hooks#7093
perf(solid-router): drop deep equality in selector hooks#7093
Conversation
📝 WalkthroughWalkthroughRemoved Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit f58f442
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/solid-router/src/useMatch.tsx (1)
109-116: Skip the memo in the defaultuseMatch()path.After this change, the
!opts.selectbranch is only forwardingmatch(). Returningmatchdirectly would align this hook with the existing fast paths inuseLocation()/useRouterState()and keep the common case off an extra memo node.♻️ Suggested refactor
Solid.createEffect(() => { if (match() !== undefined) { return } @@ invariant() } }) + if (!opts.select) { + return match as Solid.Accessor< + ThrowOrOptional<UseMatchResult<TRouter, TFrom, TStrict, TSelected>, TThrow> + > + } + return Solid.createMemo(() => { const selectedMatch = match() if (selectedMatch === undefined) return undefined - return (opts.select ? opts.select(selectedMatch as any) : selectedMatch) as - | TSelected - | undefined + return opts.select(selectedMatch as any) as TSelected | undefined }) as any }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/solid-router/src/useMatch.tsx` around lines 109 - 116, The current useMatch() always wraps the result in Solid.createMemo, but when opts.select is falsy we should skip creating an extra memo and return match() directly; update the implementation in useMatch to check if opts.select is provided and only call Solid.createMemo when it is, otherwise return match() (cast to the expected type) so the fast path avoids the extra memo node and aligns with useLocation()/useRouterState() behavior.packages/solid-router/src/Matches.tsx (1)
217-222: Fast-path the defaultuseMatches()case.This still allocates a memo even when no selector is provided, while
useLocation()anduseRouterState()already expose the underlying store accessor directly. Given the perf goal of this PR, it seems worth skipping that extra reactive node here too.♻️ Suggested refactor
export function useMatches< TRouter extends AnyRouter = RegisteredRouter, TSelected = unknown, >( opts?: UseMatchesBaseOptions<TRouter, TSelected>, ): Solid.Accessor<UseMatchesResult<TRouter, TSelected>> { const router = useRouter<TRouter>() + if (!opts?.select) { + return (() => router.stores.activeMatchesSnapshot.state) as Solid.Accessor< + UseMatchesResult<TRouter, TSelected> + > + } + + const select = opts.select return Solid.createMemo(() => { const matches = router.stores.activeMatchesSnapshot.state as Array< MakeRouteMatchUnion<TRouter> > - return (opts?.select ? opts.select(matches) : matches) as any + return select(matches) as UseMatchesResult<TRouter, TSelected> }) as Solid.Accessor<UseMatchesResult<TRouter, TSelected>> }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/solid-router/src/Matches.tsx` around lines 217 - 222, The current useMatches implementation always wraps the store accessor in Solid.createMemo which allocates an unnecessary reactive node when no selector is passed; change useMatches to fast-path the default case by returning the underlying store accessor directly when opts?.select is falsy (i.e. return a Solid.Accessor that reads router.stores.activeMatchesSnapshot.state and casts to UseMatchesResult/MakeRouteMatchUnion) and only call Solid.createMemo when opts?.select is provided (apply the selector to the matches inside the memo). Target symbols: useMatches, Solid.createMemo, router.stores.activeMatchesSnapshot.state, opts?.select, UseMatchesResult, MakeRouteMatchUnion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/solid-router/src/useRouterState.tsx`:
- Around line 56-58: The change in useRouterState (and the identical changes in
useLocation and useMatch) removed the previous-value structural sharing via
replaceEqualDeep causing selectors that allocate new objects/arrays to always
invalidate downstream; update the release notes documenting this as a
breaking/behavioral change and add regression tests that assert selector
stability (not just call counts) — modify or add tests alongside
store-updates-during-navigation.test.tsx to verify that selectors returning
structurally-equal values do not cause re-renders, and/or restore/offer an
opt-in to replaceEqualDeep behavior in useRouterState/useLocation/useMatch by
referencing the replaceEqualDeep logic so consumers can opt back into
deep-equality sharing if required.
---
Nitpick comments:
In `@packages/solid-router/src/Matches.tsx`:
- Around line 217-222: The current useMatches implementation always wraps the
store accessor in Solid.createMemo which allocates an unnecessary reactive node
when no selector is passed; change useMatches to fast-path the default case by
returning the underlying store accessor directly when opts?.select is falsy
(i.e. return a Solid.Accessor that reads
router.stores.activeMatchesSnapshot.state and casts to
UseMatchesResult/MakeRouteMatchUnion) and only call Solid.createMemo when
opts?.select is provided (apply the selector to the matches inside the memo).
Target symbols: useMatches, Solid.createMemo,
router.stores.activeMatchesSnapshot.state, opts?.select, UseMatchesResult,
MakeRouteMatchUnion.
In `@packages/solid-router/src/useMatch.tsx`:
- Around line 109-116: The current useMatch() always wraps the result in
Solid.createMemo, but when opts.select is falsy we should skip creating an extra
memo and return match() directly; update the implementation in useMatch to check
if opts.select is provided and only call Solid.createMemo when it is, otherwise
return match() (cast to the expected type) so the fast path avoids the extra
memo node and aligns with useLocation()/useRouterState() behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68171639-179c-4456-b083-69f993b3e673
📒 Files selected for processing (4)
packages/solid-router/src/Matches.tsxpackages/solid-router/src/useLocation.tsxpackages/solid-router/src/useMatch.tsxpackages/solid-router/src/useRouterState.tsx
| return Solid.createMemo(() => | ||
| select(router.stores.__store.state), | ||
| ) as Accessor<UseRouterStateResult<TRouter, TSelected>> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "useRouterState.tsx" | head -20Repository: TanStack/router
Length of output: 197
🏁 Script executed:
git ls-files | grep -E "solid-router.*src" | head -30Repository: TanStack/router
Length of output: 2079
🏁 Script executed:
cat -n packages/solid-router/src/useRouterState.tsx 2>/dev/null | head -100Repository: TanStack/router
Length of output: 2294
🏁 Script executed:
cd packages/solid-router && git log --oneline -20 -- src/useRouterState.tsxRepository: TanStack/router
Length of output: 125
🏁 Script executed:
cd packages/solid-router && git diff HEAD~5..HEAD -- src/useRouterState.tsx 2>/dev/null | head -100Repository: TanStack/router
Length of output: 41
🏁 Script executed:
rg -l "useRouterState|useRouterStateShallow|useSearch|useParams" packages/solid-router/src/ --type ts --type tsxRepository: TanStack/router
Length of output: 86
🏁 Script executed:
rg "replaceEqualDeep|structural.*shar|createMemo" packages/solid-router/src/ -A 2 -B 2Repository: TanStack/router
Length of output: 17448
🏁 Script executed:
find packages/solid-router -name "*.test.*" -o -name "*.spec.*" | head -10Repository: TanStack/router
Length of output: 558
🏁 Script executed:
cd packages/solid-router && git show f58f442a6:src/useRouterState.tsx | head -80Repository: TanStack/router
Length of output: 276
🏁 Script executed:
cd packages/solid-router && git show f58f442a6 -- src/useRouterState.tsx src/useLocation.tsx src/useMatch.tsxRepository: TanStack/router
Length of output: 7048
🏁 Script executed:
cd packages/solid-router && git show f58f442a6^:src/useRouterState.tsx 2>/dev/null | head -100Repository: TanStack/router
Length of output: 41
🏁 Script executed:
cd packages/solid-router && git show f58f442a6^:src/useLocation.tsx 2>/dev/null | head -100Repository: TanStack/router
Length of output: 41
🏁 Script executed:
cd packages/solid-router && git log --oneline -1 f58f442a6^Repository: TanStack/router
Length of output: 251
🏁 Script executed:
cd packages/solid-router && git log --all --oneline -- src/useRouterState.tsx src/useLocation.tsx src/useMatch.tsx | head -10Repository: TanStack/router
Length of output: 716
🏁 Script executed:
cd packages/solid-router && git log --all --oneline | head -20Repository: TanStack/router
Length of output: 1227
🏁 Script executed:
rg "useRouterStateShallow|useSearchParams|useParams" packages/solid-router/src/ -lRepository: TanStack/router
Length of output: 253
🏁 Script executed:
cat packages/solid-router/src/index.tsx | head -100Repository: TanStack/router
Length of output: 1987
🏁 Script executed:
cd packages/solid-router && git show c9e18555f:src/useRouterState.tsx 2>/dev/null | head -100Repository: TanStack/router
Length of output: 41
🏁 Script executed:
cd packages/solid-router && git log --all --format="%H %s" -- src/useRouterState.tsx src/useLocation.tsx src/useMatch.tsx | head -20Repository: TanStack/router
Length of output: 1026
🏁 Script executed:
cd packages/solid-router && git show c9e18555f --stat | grep -E "useRouterState|useLocation|useMatch"Repository: TanStack/router
Length of output: 254
🏁 Script executed:
rg "replaceEqualDeep" packages/solid-router/src/ -B 3 -A 3Repository: TanStack/router
Length of output: 1369
🏁 Script executed:
cd packages/solid-router && git show c9e18555f:src/useRouterState.tsx 2>/dev/nullRepository: TanStack/router
Length of output: 43
🏁 Script executed:
cd packages/solid-router && git diff c9e18555f f58f442a6 -- src/useRouterState.tsx src/useLocation.tsx src/useMatch.tsx | head -200Repository: TanStack/router
Length of output: 2962
🏁 Script executed:
cat packages/solid-router/tests/*.test.tsx | grep -A 10 -B 5 "useRouterState\|selector" | head -100Repository: TanStack/router
Length of output: 1125
🏁 Script executed:
cd packages/solid-router && git show c9e18555f:src/useRouterState.tsx 2>/dev/null || echo "File may have been newly created in this commit"Repository: TanStack/router
Length of output: 106
🏁 Script executed:
cd packages/solid-router && git log --all --format="%H %s" -- src/useRouterState.tsx | head -5Repository: TanStack/router
Length of output: 540
🏁 Script executed:
cd packages/solid-router && git diff 054523900 c9e18555f -- src/useRouterState.tsx | head -100Repository: TanStack/router
Length of output: 41
🏁 Script executed:
rg "useRouterState.*select|selector" packages/solid-router/tests/ -A 5 -B 2Repository: TanStack/router
Length of output: 846
🏁 Script executed:
cd packages/solid-router && git show 054523900:src/useRouterState.tsx 2>/dev/null | head -80Repository: TanStack/router
Length of output: 41
🏁 Script executed:
find . -name "*.changeset" -o -name "CHANGELOG*" | xargs grep -l "solid-router\|selector" 2>/dev/null | head -5Repository: TanStack/router
Length of output: 274
🏁 Script executed:
cat .changeset/*.md 2>/dev/null | grep -A 10 -B 2 "solid-router\|selector\|deep.*equal" | head -80Repository: TanStack/router
Length of output: 41
🏁 Script executed:
cd packages/solid-router && git log --all --oneline | grep -i "selector\|deep.*equal\|shallow" | head -10Repository: TanStack/router
Length of output: 248
🏁 Script executed:
cd packages/solid-router && git show 054523900:src/useRouterState.tsx 2>/dev/null | tail -50Repository: TanStack/router
Length of output: 41
🏁 Script executed:
ls -la .changeset/ 2>/dev/null | head -20Repository: TanStack/router
Length of output: 230
🏁 Script executed:
git log --all --oneline f58f442a6^..f58f442a6 2>/dev/null | head -5Repository: TanStack/router
Length of output: 41
🏁 Script executed:
cat packages/solid-router/CHANGELOG.md | head -100Repository: TanStack/router
Length of output: 3184
🏁 Script executed:
cd packages/solid-router && git log --all --oneline --grep="selector\|deep.*equal" | head -10Repository: TanStack/router
Length of output: 248
🏁 Script executed:
rg "Solid.createMemo" packages/solid-router/src/ | grep -E "prev|equal|shallow" | head -20Repository: TanStack/router
Length of output: 180
🏁 Script executed:
cat packages/solid-router/tests/store-updates-during-navigation.test.tsxRepository: TanStack/router
Length of output: 8711
🏁 Script executed:
rg "breaking\|BREAKING\|v[0-9]" packages/solid-router/CHANGELOG.md | head -20Repository: TanStack/router
Length of output: 41
🏁 Script executed:
cd packages/solid-router && git log --format="%B" -1 f58f442a6Repository: TanStack/router
Length of output: 116
🏁 Script executed:
rg "selector\|shallow\|deep.*equal" packages/solid-router/CHANGELOG.md | head -20Repository: TanStack/router
Length of output: 41
Document the selector invalidation semantics change before merging.
Lines 56–58 remove the previous-value structural sharing that used replaceEqualDeep. This means selectors allocating fresh objects or arrays will now trigger downstream invalidations on every router-store update, even when selected fields are unchanged. This is an observable API contract change, so it requires:
- Explicit documentation in the release notes as a breaking change or major behavioral shift
- Regression tests confirming selector stability behavior (the existing call-count assertions in
store-updates-during-navigation.test.tsxverify invocation counts, not stability)
The same concern applies to useLocation and useMatch, which have identical changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/solid-router/src/useRouterState.tsx` around lines 56 - 58, The
change in useRouterState (and the identical changes in useLocation and useMatch)
removed the previous-value structural sharing via replaceEqualDeep causing
selectors that allocate new objects/arrays to always invalidate downstream;
update the release notes documenting this as a breaking/behavioral change and
add regression tests that assert selector stability (not just call counts) —
modify or add tests alongside store-updates-during-navigation.test.tsx to verify
that selectors returning structurally-equal values do not cause re-renders,
and/or restore/offer an opt-in to replaceEqualDeep behavior in
useRouterState/useLocation/useMatch by referencing the replaceEqualDeep logic so
consumers can opt back into deep-equality sharing if required.
1 participant
DO NOT MERGE, I JUST OPENED THE PR TO GET CODSPEED MEASUREMENTS
Summary
replaceEqualDeepfrom the hot-path Solid selector hooks inuseRouterState,useLocation,useMatch, anduseMatches@benchmarks/client-navSolid benchmarkTesting
CI=1 NX_DAEMON=false pnpm nx run @tanstack/solid-router:test:unit --outputStyle=stream --skipRemoteCacheCI=1 NX_DAEMON=false pnpm nx run @tanstack/solid-router:test:types --outputStyle=stream --skipRemoteCacheCI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:solid --outputStyle=stream --skipRemoteCacheSummary by CodeRabbit
useMatches,useLocation,useMatch,useRouterState) by streamlining internal value stabilization. No changes to public APIs or user-facing behavior.