fix(react-router, solid-router) - useParams should only be concerned about the relevant params #5097
Conversation
WalkthroughAdds a RenderCounter component; replaces a leaf /params-ps/named/$foo route with a nested $foo → $bar → $baz hierarchy in React and Solid e2e apps; updates generated route trees and public typings; changes useParams internal selection to respect strict vs non-strict params; and adds/updates tests for param stability and render counts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor C as Component
participant UP as useParams(opts)
participant UM as useMatch()
participant R as Router
Note over C,UP: Params resolution chooses strict vs non-strict params
C->>UP: call useParams({ strict?, select? })
UP->>UM: useMatch({ strict: opts.strict, select: (m => m) })
alt strict mode
UM-->>UP: matchId
UP->>R: getMatch(matchId)
R-->>UP: { _strictParams }
UP-->>C: return opts.select? opts.select(_strictParams) : _strictParams
else non-strict
UM-->>UP: match
UP-->>C: return opts.select? opts.select(match.params) : match.params
end
sequenceDiagram
autonumber
actor U as User
participant App as Router App
participant Foo as /params-ps/named/$foo
participant Bar as /params-ps/named/$foo/$bar
participant Baz as /params-ps/named/$foo/$bar/$baz
U->>App: Navigate /params-ps/named/foo
App->>Foo: mount/render
Foo-->>U: render links (index, ./$bar?bar=1, ./$bar?bar=2)
U->>Foo: Click "./$bar?bar=1"
Foo->>Bar: mount/render (RenderCounter increments)
Bar-->>U: render link to ./$baz (baz = "1_10")
U->>Bar: Click "./$baz?baz=1_10"
Bar->>Baz: mount/render
Baz-->>U: show baz value (data-testid="foo-bar-baz-value")
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
⏰ 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). (2)
🔇 Additional comments (3)
✨ Finishing Touches
🧪 Generate unit tests
Comment |
|
View your CI Pipeline Execution ↗ for commit 2be5eba
☁️ 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: |
nlynzaad
changed the title
nlynzaad
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (15)
packages/react-router/tests/router.test.tsx (3)
934-955: Fix label typo for link-foo2-2-bar2-1.
The link’s params use bar2Id='1' but the label says Bar2_2.Apply:
- <Link + <Link data-testid="link-foo2-2-bar2-1" to="/foo2/$foo2Id/bar2/$bar2Id" params={{ foo2Id: '2', bar2Id: '1' }} > - Foo2-2-Bar2_2 + Foo2-2-Bar2_1 </Link>
1018-1036: Minor consistency: “mount” vs effect-run semantics.
These useEffects depend on foo2Id to count effect runs, not mounts (unlike the fooId test using []). Either rename the mock to reflect “effect count” or add a short comment to avoid confusion.
1136-1160: LGTM: solid coverage of parent stability and render-count behavior.
Optional: add a counterpart asserting re-render behavior with useParams({ strict: false }) to lock in the fallback semantics.e2e/react-router/basic-file-based/src/components/RenderCounter.tsx (1)
3-7: LGTM, with a tiny StrictMode caveatThis works for tracking renders. Note: in React 18 StrictMode (dev), counts will double on mount. Fine for prod/E2E, but keep in mind for local debugging.
e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.$baz.tsx (1)
8-13: Optional: avoid destructuring to keep reactivity intactCurrent usage
params().foois correct for Solid. Avoidconst { foo } = params()as it breaks reactivity.packages/react-router/src/useParams.tsx (1)
91-91: Plumb through shouldThrow to preserve API semantics
useParamsexposesshouldThrow, but the call hard-codesshouldThrow: false. Consider honoring the option.- shouldThrow: false, + shouldThrow: (opts as any).shouldThrow ?? false,If the intended contract is “never throw, return {}”, ignore this, but please confirm.
e2e/react-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.$baz.tsx (1)
11-13: Fix greeting interpolation (currently renders literal “{foo}”).Interpolate with a template literal so the values appear.
- Hello "/params-ps/named/{foo}/{bar}/{baz}"! + {`Hello "/params-ps/named/${foo}/${bar}/${baz}"!`}e2e/react-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.route.tsx (1)
20-21: Fix greeting interpolation (currently renders literal “{foo}”, “{bar}”).- Hello "/params-ps/named/{foo}/{bar}"! + {`Hello "/params-ps/named/${foo}/${bar}"!`}e2e/react-router/basic-file-based/tests/params.spec.ts (1)
133-205: Great coverage of render stability; minor flake-proofing suggestion.Consider
await expect(page).toHaveURL(path)instead ofnetworkidle, and prefertoBeVisible()overtoBeInViewport()for CI stability.e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/route.tsx (1)
9-9: Rename setter for clarity (typo).- const [renderCount, setRenderCountCount] = createSignal(0) + const [renderCount, setRenderCount] = createSignal(0) @@ - setRenderCountCount((prev) => prev + 1) + setRenderCount((prev) => prev + 1)Also applies to: 15-16
e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.route.tsx (3)
14-16: Nit: fix setter names for clarity.Typo-ish double “Count”. Improves readability.
-const [renderBarCount, setBarRenderCountCount] = createSignal(0) -const [renderBazCount, setBazRenderCountCount] = createSignal(0) +const [renderBarCount, setBarRenderCount] = createSignal(0) +const [renderBazCount, setBazRenderCount] = createSignal(0)
21-29: Optional: count only on param change (skip initial run).If you want the counters to increment only when bar/baz actually change (not on mount), wrap with on(..., { defer: true }). Current behavior aligns with tests, so change only if desired.
-import { createEffect, createSignal } from 'solid-js' +import { createEffect, createSignal, on } from 'solid-js' @@ -createEffect(() => { - params().bar - setBarRenderCountCount((prev) => prev + 1) -}) +createEffect( + on(() => params().bar, () => setBarRenderCount((p) => p + 1), { + defer: true, + }), +) @@ -createEffect(() => { - params().baz - setBazRenderCountCount((prev) => prev + 1) -}) +createEffect( + on(() => params().baz, () => setBazRenderCount((p) => p + 1), { + defer: true, + }), +)
53-58: Optional: memoize computed baz param.Prevents reformat churn in JSX and clarifies intent.
+ const bazParam = () => `${params().bar}_10` @@ - params={{ baz: `${params().bar}_10` }} + params={{ baz: bazParam() }}e2e/solid-router/basic-file-based/tests/params.spec.ts (2)
117-124: Harden navigation assertions to avoid flakiness.SPA navigations may not always trigger networkidle. Assert URL after clicks.
await fooBar2Link.click() + await expect(page).toHaveURL(/\/params-ps\/named\/foo\/2$/) await expect(fooValue).toBeInViewport() await expect(fooRenderCount).toBeInViewport() await expect(fooBarValue).toBeInViewport()
125-131: Symmetry: also assert URL when going back to index.Reduces timing flakes.
await fooIndexLink.click() + await expect(page).toHaveURL('/params-ps/named/foo') await expect(fooValue).toBeInViewport() await expect(fooRenderCount).toBeInViewport() await expect(fooBarValue).not.toBeInViewport()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
e2e/react-router/basic-file-based/src/components/RenderCounter.tsx(1 hunks)e2e/react-router/basic-file-based/src/routeTree.gen.ts(22 hunks)e2e/react-router/basic-file-based/src/routes/params-ps/named/$foo.tsx(0 hunks)e2e/react-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.$baz.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.route.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/params-ps/named/$foo/route.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/params-ps/named/index.tsx(1 hunks)e2e/react-router/basic-file-based/tests/params.spec.ts(1 hunks)e2e/solid-router/basic-file-based/src/routeTree.gen.ts(22 hunks)e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.$baz.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.route.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/route.tsx(1 hunks)e2e/solid-router/basic-file-based/tests/params.spec.ts(1 hunks)packages/react-router/src/useParams.tsx(2 hunks)packages/react-router/tests/router.test.tsx(7 hunks)packages/solid-router/src/useParams.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- e2e/react-router/basic-file-based/src/routes/params-ps/named/$foo.tsx
🧰 Additional context used
🧬 Code graph analysis (9)
e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/route.tsx (2)
e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.$baz.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.route.tsx (1)
Route(9-11)
e2e/react-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.route.tsx (4)
e2e/react-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.$baz.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/params-ps/named/$foo/route.tsx (1)
Route(4-6)e2e/react-router/basic-file-based/src/routes/params-ps/named/index.tsx (1)
Route(3-7)e2e/react-router/basic-file-based/src/components/RenderCounter.tsx (1)
RenderCounter(3-7)
packages/react-router/src/useParams.tsx (2)
packages/react-router/src/useRouter.tsx (1)
useRouter(6-15)packages/react-router/src/useMatch.tsx (1)
useMatch(78-119)
e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.route.tsx (2)
e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.$baz.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/route.tsx (1)
Route(4-6)
e2e/react-router/basic-file-based/src/routes/params-ps/named/$foo/route.tsx (4)
e2e/react-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.$baz.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.route.tsx (1)
Route(9-11)e2e/react-router/basic-file-based/src/routes/params-ps/named/index.tsx (1)
Route(3-7)e2e/react-router/basic-file-based/src/components/RenderCounter.tsx (1)
RenderCounter(3-7)
packages/solid-router/src/useParams.tsx (2)
packages/solid-router/src/useRouter.tsx (1)
useRouter(6-15)packages/solid-router/src/useMatch.tsx (1)
useMatch(55-96)
packages/react-router/tests/router.test.tsx (2)
packages/react-router/src/Match.tsx (1)
Outlet(309-359)packages/react-router/src/route.tsx (1)
createRoute(293-358)
e2e/react-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.$baz.tsx (3)
e2e/react-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.route.tsx (1)
Route(9-11)e2e/react-router/basic-file-based/src/routes/params-ps/named/$foo/route.tsx (1)
Route(4-6)e2e/react-router/basic-file-based/src/routes/params-ps/named/index.tsx (1)
Route(3-7)
e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.$baz.tsx (2)
e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.route.tsx (1)
Route(9-11)e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/route.tsx (1)
Route(4-6)
⏰ 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). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (25)
packages/react-router/tests/router.test.tsx (11)
1-1: LGTM: added useRef import is appropriate for the render counter.
885-891: LGTM: extended remountDeps types to include foo2Id/bar2Id.
This keeps typings aligned with the new nested routes.
893-898: LGTM: added foo2Id/bar2Id mount mocks.
Matches the new routes and checks.
996-1001: Bugfix LGTM: BarIdRouteComponent now reads barId from the correct route.
This removes cross-route coupling.
1045-1057: LGTM: Bar2Id route/component and useParams usage look correct.
1059-1063: LGTM: added foo2IdRoute subtree to the route tree.
1077-1080: LGTM: queries for new foo2/bar2 links.
1085-1088: LGTM: presence assertions for new links.
1089-1101: LGTM: exposing new links from setup().
1105-1108: LGTM: extended page union and mountMocks typing.
1543-1552: LGTM: using ['~standard'] brand for StandardSchemaValidator test case.
Pattern matches the library’s expected shape.e2e/react-router/basic-file-based/src/routes/params-ps/named/index.tsx (1)
1-7: LGTM: redirect index route is clear and matches file-based pathImport merge and the beforeLoad redirect look correct for this index route.
packages/react-router/src/useParams.tsx (1)
89-95: Good: stable selection in strict mode to prevent parent re-rendersSelecting
match.idwhen strict yields a stable subscription target—nice way to avoid needless updates.packages/solid-router/src/useParams.tsx (1)
67-74: Acknowledged.e2e/react-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.route.tsx (1)
14-17: LGTM: strict: false here correctly opts into bubbling child params to parent.e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/route.tsx (1)
12-16: LGTM: effect tracks only foo; render count stays stable across child param changes.e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.route.tsx (1)
17-19: Confirm intentional non-strict params usage.strict: false exposes child params (baz) to this route; matches test intent. Just confirming this is deliberate.
e2e/react-router/basic-file-based/src/routes/params-ps/named/$foo/route.tsx (1)
8-21: LGTM: route uses params scoped to its own match (stable renders).Matches strict-by-default behavior and test expectations.
e2e/solid-router/basic-file-based/src/routeTree.gen.ts (1)
54-67: Generated tree looks consistent.Imports, ids/paths, fullPaths, and parent-child wiring for $foo → $bar → $baz are coherent.
Also applies to: 280-356, 377-406, 426-455, 483-512, 537-566, 586-615, 642-671, 691-697, 1230-1281
e2e/react-router/basic-file-based/src/routeTree.gen.ts (6)
64-64: LGTM: Nested param route imports are consistent and expected.The three imports correctly reflect the new nested structure and naming conventions for dynamic segments.
Also applies to: 71-71, 76-76
346-350: LGTM: Route instances created and parented correctly.
/$foois rooted atrootRouteImport./$barcorrectly parents under/$foowith relative path/$bar./$bazcorrectly parents under/$barwith relative path/$baz.Also applies to: 385-390, 415-419
446-446: LGTM: Public maps and unions updated for new nested paths.
FileRoutesByFullPath,FileRoutesByTo,FileRoutesById, andFileRouteTypesunions now include/$foo,/$foo/$bar, and/$foo/$bar/$bazwith the correctWithChildrentypes where applicable.Also applies to: 470-470, 477-477, 505-505, 529-529, 536-536, 572-572, 596-596, 603-603, 636-636, 660-660, 667-667, 695-695, 719-719, 726-726, 761-761, 785-785, 792-792
820-820: LGTM: Root route children wiring uses WithChildren wrappers.
ParamsPsNamedFooRouteRouteis exposed via itsWithChildrenvariant in both the interface and the instantiated children map.Also applies to: 1485-1485
1194-1200: LGTM: FileRoutesByPath entries are correct.
path,fullPath, andparentRoutevalues are consistent:
/$foois top-level under root with absolutepath./$barand/$bazuse relativepathsegments and correct parents.Also applies to: 1243-1249, 1278-1284
1436-1449: LGTM: Children wrappers for nested segments are properly declared.Interfaces, constants, and
_addFileChildrenwiring for/$bar→/$bazand/$foo→/$barare set up correctly.Also applies to: 1450-1462
e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.$baz.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/react-router/src/useParams.tsx (1)
89-101: Prevent undefined access in non-strict mode and avoid calling getMatch with undefined; also respect shouldThrow and pass a safe fallback into opts.select
- Non-strict path can NPE when no match (matchResult undefined → matchResult.params).
- Strict path may call router.getMatch(undefined).
- Current code ignores opts.shouldThrow and always sets shouldThrow: false.
- opts.select receives undefined today; pass {} instead.
Apply:
- const matchResult = useMatch({ - from: opts.from!, - shouldThrow: false, - structuralSharing: opts.structuralSharing, - strict: opts.strict, - select: (match) => (isStrict ? match.id : match), - }) as any + const matchResult = useMatch({ + from: opts.from, + shouldThrow: (opts.shouldThrow as any), + structuralSharing: opts.structuralSharing, + strict: opts.strict, + select: (match) => (isStrict ? match.id : match), + }) as any @@ - const params = isStrict - ? router.getMatch(matchResult)?.params - : matchResult.params + const params = isStrict + ? (matchResult ? router.getMatch(matchResult)?.params : undefined) + : matchResult?.params @@ - return opts.select ? (opts.select(params) as any) : (params ?? {}) + const base = params ?? ({} as any) + return opts.select ? (opts.select(base) as any) : baseTests: add cases for (1) from missing with shouldThrow: false → {}, (2) from missing with shouldThrow: true → throws, for both strict true/false.
#!/bin/bash # Scan for useParams call sites that rely on shouldThrow semantics rg -nP --type=ts -C2 '\buseParams\s*<[^>]*>\s*\(\s*{[^}]*shouldThrow\s*:\s*true' || true # Quick sanity on this file’s branches rg -n 'useParams\\.|useMatch\\(|shouldThrow|strict:' packages/react-router/src/useParams.tsx -n -C2
🧹 Nitpick comments (4)
e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.$baz.tsx (2)
11-11: Nit: render the path as code instead of a quoted stringMinor readability tweak; using
<code>communicates “path literal” intent and avoids nested quotes noise.- Hello "/params-ps/named/$foo/$bar/$baz"! + Hello <code>/params-ps/named/$foo/$bar/$baz</code>!
12-14: Optionally expose all params for easier E2E assertionsSince this route exercises nested params, consider rendering
fooandbaralongsidebazwith testids to simplify assertions and parity with sibling routes.<div> - baz: <span data-testid="foo-bar-baz-value">{params().baz}</span> + foo: <span data-testid="foo-value">{params().foo}</span> + </div> + <div> + bar: <span data-testid="foo-bar-value">{params().bar}</span> + </div> + <div> + baz: <span data-testid="foo-bar-baz-value">{params().baz}</span> </div>packages/react-router/src/useParams.tsx (2)
90-90: Avoid unnecessary non-null assertion on fromPassing opts.from is fine; the current non-null assertion is redundant and can hide issues at compile time.
101-101: Optional: return a stable empty object to reduce needless rerenders when unmatchedUsing a shared EMPTY_PARAMS avoids creating a new {} each render in the unmatched case.
Outside this hunk:
const EMPTY_PARAMS: Record<string, string> = Object.freeze({})Then:
- const base = params ?? ({} as any) + const base = params ?? (EMPTY_PARAMS as any)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.$baz.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.route.tsx(1 hunks)packages/react-router/src/useParams.tsx(2 hunks)packages/react-router/tests/router.test.tsx(7 hunks)packages/solid-router/src/useParams.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.route.tsx
- packages/react-router/tests/router.test.tsx
- packages/solid-router/src/useParams.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.$baz.tsx (2)
e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.route.tsx (1)
Route(9-11)e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/route.tsx (1)
Route(4-6)
packages/react-router/src/useParams.tsx (2)
packages/react-router/src/useRouter.tsx (1)
useRouter(6-15)packages/react-router/src/useMatch.tsx (1)
useMatch(78-119)
⏰ 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). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (2)
e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.$baz.tsx (2)
8-14: Solid accessor usage is correct and stableUsing
Route.useParams()as an accessor (params().baz) matches Solid’s reactive pattern and the new strict defaults. Looks good.
3-5: Route Mapping Confirmed:routeTree.gen.tsimports the component from./routes/params-ps/named/$foo/$bar.$baz, setsid/pathto/$bazwith parentParamsPsNamedFooBarRouteRoute, and registers/params-ps/named/$foo/$bar/$bazin the route map. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/react-router/src/useParams.tsx (1)
97-101: Strict mode must return route-only params and be null-safe (use _strictParams; avoid crashes when no match).Currently strict mode reads
match.params(the merged params) and assumes a match always exists. This violates the PR intent (parent should not see child params) and can throw whenshouldThrowis false and no match is found. Return_strictParamsand guard undefined before callingrouter.getMatch.- const params = isStrict - ? router.getMatch(matchResult)?.params - : matchResult.params - - return opts.select ? (opts.select(params) as any) : (params ?? {}) + const params = isStrict + ? (matchResult ? router.getMatch(matchResult)?._strictParams : undefined) + : matchResult?.params + + return opts.select + ? ((opts.select((params ?? ({} as any)) as any) as any)) + : (params ?? {})
🧹 Nitpick comments (3)
packages/react-router/src/useParams.tsx (1)
89-95: Prefer passing the resolved boolean touseMatchand keep the selection minimal.Pass
strict: isStrictto avoid forwardingundefined, and keep the select focused onidonly when strict.- const matchResult = useMatch({ + const matchResult = useMatch({ from: opts.from!, shouldThrow: opts.shouldThrow, structuralSharing: opts.structuralSharing, - strict: opts.strict, + strict: isStrict, select: (match) => (isStrict ? match.id : match), }) as anypackages/react-router/tests/router.test.tsx (2)
1018-1036: Strengthen the contract: assert strictuseParamsreturns only own params.Add an assertion that
foo2IdRoute.useParams()does not exposebar2Idto the parent route to prevent regressions.Would you like me to add a focused test like:
it('foo2 route sees only its own params in strict mode', async () => { const { router } = await setup() await act(() => fireEvent.click((await screen.findByTestId('link-foo2-1-bar2-1')))) const fooEl = await screen.findByTestId('foo2Id-page') // Inject a helper span in Foo2IdRouteComponent that renders Object.keys(useParams()) // and assert it equals ['foo2Id']. })I can wire this by rendering
Object.keys(foo2IdRoute.useParams()).join(',')into a hidden element inFoo2IdRouteComponentand asserting it staysfoo2Idwhen navigating.
1136-1160: Minor test naming: "mount" mocks also count updates.
useEffect(..., [foo2Id])anduseEffect(..., [bar2Id])track param changes, not just mounts. Consider renaming torenderDeps/paramChangeMocksfor clarity to future readers. No functional change required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.route.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/route.tsx(1 hunks)packages/react-router/src/useParams.tsx(2 hunks)packages/react-router/tests/router.test.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/$bar.route.tsx
- e2e/solid-router/basic-file-based/src/routes/params-ps/named/$foo/route.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
packages/react-router/tests/router.test.tsx (2)
packages/react-router/src/Match.tsx (1)
Outlet(309-359)packages/react-router/src/route.tsx (1)
createRoute(293-358)
packages/react-router/src/useParams.tsx (2)
packages/react-router/src/useRouter.tsx (1)
useRouter(6-15)packages/react-router/src/useMatch.tsx (1)
useMatch(78-119)
⏰ 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). (2)
- GitHub Check: Preview
- GitHub Check: Test
1 participant
#5020 highlighted an issue where when using useParams in parent route (that has its own path param) with child routes that also use path params this can cause excessive re-rendering of the parent route when the params for the child routes changes.
The root cause for this is that the child params are available to the parent route, even when strict=false was not specified and this also does not match to the types that is being returned in this case. This can be worked around by using select inside the useParams hook/function but results in unnecessary boilerplate.
This PR attempts to resolve the issue by dealing with the two distinct cases, one where we are only concerned about the path params of the current route and prior routes (strict=true/undefined) and the second where we want access in the parent route to the path params of the child routes (strict=false).
In the strict case we use the match id to return the params of the specific match, which is constant and results in stable rendering, while in the strict=false case we fallback to the initial implementation of returning params from the useMatch result which can be variable and requires re-rendering to be done.
Changes were done on useParams for both react and solid, in solid's case this wasn't causing re-renders but did return unexpected child params, the change for solid is more toward keeping the API and expected outputs consistent between the two frameworks. existing unit tests for react was updated and e2e tests for both frameworks was added to ensure rendering and results match the expected.
A separate merge has been prepared for Alpha and will be merged on signoff of this one.
Summary by CodeRabbit
New Features
Bug Fixes
Tests