fix(router-core): _strictParams and path defining characters #5135
Conversation
WalkthroughAdds a non-nested param routes subtree under /params-ps for React and Solid E2E apps, updates generated route trees and tests, and changes RouterCore.matchRoutesInternal to derive strictParams from fullPath interpolation (usedParams). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant R as RouterCore.matchRoutesInternal
participant P as interpolatePath
participant RT as RouteTree
U->>R: Navigate to /params-ps/non-nested/foo/bar
R->>P: interpolate(route.fullPath, incomingParams)
P-->>R: { interpolatedPath, usedParams }
Note right of R: strictParams := usedParams
R->>P: interpolate(route.id, leaveWildcards=true)
P-->>R: { interpolatedPath: matchId }
R->>RT: build match (pathname from fullPath, id from matchId)
U->>R: useParams({ from: '/params-ps/non-nested/$foo_' })
R-->>U: { foo: 'foo' }
U->>R: Route.useParams() at leaf /$foo_/$bar
R-->>U: { foo: 'foo', bar: 'bar' }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧬 Code graph analysis (1)e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/route.tsx (3)
⏰ 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)
🔇 Additional comments (2)
Pre-merge checks (4 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
View your CI Pipeline Execution ↗ for commit 8a5da44
☁️ 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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/router-core/src/router.ts (1)
1186-1191: Optional: pass parseCache to interpolatePath(route.fullPath) for consistency and perf.
Keeps parsing cached like the matchId call.Apply:
- const { interpolatedPath, usedParams } = interpolatePath({ + const { interpolatedPath, usedParams } = interpolatePath({ path: route.fullPath, params: routeParams, decodeCharMap: this.pathParamsDecodeCharMap, + parseCache: this.parsePathnameCache, })e2e/solid-router/basic-file-based/tests/params.spec.ts (2)
139-140: Stabilize waitForURL matching: use a glob patternThe literal string may not match the full absolute URL. Prefer a glob to avoid flakiness.
- await page.waitForURL('/params-ps/non-nested') + await page.waitForURL('**/params-ps/non-nested')
152-160: Prefer textContent() and inline parse to reduce layout dependencyinnerText triggers layout and can be slower/flaky. textContent is faster and sufficient here.
- const fooParamsValue = page.getByTestId('foo-params-value') - const fooParamsText = await fooParamsValue.innerText() - const fooParamsObj = JSON.parse(fooParamsText) - expect(fooParamsObj).toEqual({ foo: 'foo' }) + const fooParamsText = await page.getByTestId('foo-params-value').textContent() + expect(JSON.parse(fooParamsText ?? '{}')).toEqual({ foo: 'foo' }) - - const paramsValue = page.getByTestId('foo-bar-params-value') - const paramsText = await paramsValue.innerText() - const paramsObj = JSON.parse(paramsText) - expect(paramsObj).toEqual({ foo: 'foo', bar: 'bar' }) + const paramsText = await page.getByTestId('foo-bar-params-value').textContent() + expect(JSON.parse(paramsText ?? '{}')).toEqual({ foo: 'foo', bar: 'bar' })e2e/react-router/basic-file-based/tests/params.spec.ts (1)
64-65: Stabilize waitForURL matching: use a glob patternUse a glob to match the absolute URL and avoid hangs/flakes.
- await page.waitForURL('/params-ps/non-nested') + await page.waitForURL('**/params-ps/non-nested')e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/route.tsx (1)
10-24: Move Outlet outside offor valid markup
should contain only
- children. Place after the list (or wrap in
- ).
- <ul className="grid mb-2"> - <li> - <Link - from={Route.fullPath} - data-testid="l-to-non-nested-foo-bar" - to="./$foo/$bar" - params={{ foo: 'foo', bar: 'bar' }} - > - /params-ps/non-nested/$foo/$bar - </Link> - </li> - <Outlet /> - </ul> + <ul className="grid mb-2"> + <li> + <Link + from={Route.fullPath} + data-testid="l-to-non-nested-foo-bar" + to="./$foo/$bar" + params={{ foo: 'foo', bar: 'bar' }} + > + /params-ps/non-nested/$foo/$bar + </Link> + </li> + </ul> + <Outlet />e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/route.tsx (1)
10-24: Move Outlet outside offor valid markup
Ensure list semantics; mirror the React fix.
- <ul class="grid mb-2"> - <li> - <Link - from={Route.fullPath} - data-testid="l-to-non-nested-foo-bar" - to="./$foo/$bar" - params={{ foo: 'foo', bar: 'bar' }} - > - /params-ps/non-nested/$foo/$bar - </Link> - </li> - <Outlet /> - </ul> + <ul class="grid mb-2"> + <li> + <Link + from={Route.fullPath} + data-testid="l-to-non-nested-foo-bar" + to="./$foo/$bar" + params={{ foo: 'foo', bar: 'bar' }} + > + /params-ps/non-nested/$foo/$bar + </Link> + </li> + </ul> + <Outlet />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
e2e/react-router/basic-file-based/src/routeTree.gen.ts(29 hunks)e2e/react-router/basic-file-based/src/routes/params-ps/index.tsx(2 hunks)e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/$bar.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/route.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/route.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(30 hunks)e2e/solid-router/basic-file-based/src/routes/params-ps/index.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/$bar.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/route.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/route.tsx(1 hunks)e2e/solid-router/basic-file-based/tests/params.spec.ts(1 hunks)packages/router-core/src/router.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/route.tsx (3)
e2e/react-router/basic-file-based/src/routes/params-ps/index.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/$bar.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/route.tsx (1)
Route(3-5)
e2e/solid-router/basic-file-based/src/routes/params-ps/index.tsx (3)
e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/$bar.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/route.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/route.tsx (1)
Route(3-5)
e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/$bar.tsx (3)
e2e/react-router/basic-file-based/src/routes/params-ps/index.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/route.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/route.tsx (1)
Route(3-5)
e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/route.tsx (3)
e2e/solid-router/basic-file-based/src/routes/params-ps/index.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/$bar.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/route.tsx (1)
Route(3-5)
e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/route.tsx (3)
e2e/solid-router/basic-file-based/src/routes/params-ps/index.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/$bar.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/route.tsx (1)
Route(3-5)
e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/$bar.tsx (3)
e2e/solid-router/basic-file-based/src/routes/params-ps/index.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/route.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/route.tsx (1)
Route(3-5)
packages/router-core/src/router.ts (1)
packages/router-core/src/path.ts (1)
interpolatePath(371-485)
e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/route.tsx (3)
e2e/react-router/basic-file-based/src/routes/params-ps/index.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/$bar.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/route.tsx (1)
Route(3-5)
⏰ 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 (23)
e2e/react-router/basic-file-based/src/routes/params-ps/index.tsx (2)
1-1: LGTM: consolidated import is clean and consistent.
71-79: LGTM: new “Non-nested path params” nav entry wired correctly.
Link target and data-testid match the new routes/tests.packages/router-core/src/router.ts (2)
1198-1205: LGTM: inline matchId interpolation keeps ID stability and avoids extra temp vars.
Includes decodeCharMap and parseCache; behavior preserved with loaderDepsHash.
1213-1214: Approve: strictParams now derived from fullPath usedParams — fixes underscore-suffixed params.E2E routes/links present; no interpolation-based strictParams usage found; no direct useParams({ from: '/params-ps/non-nested/$foo_' }) references.
Matches: e2e/react-router/basic-file-based/tests/params.spec.ts; e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/route.tsx; e2e/react-router/basic-file-based/src/routes/params-ps/index.tsx
e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/route.tsx (2)
3-5: LGTM: parent param route registered correctly.
Path matches the directory and pairs with the$barchild.
7-9: LGTM: Outlet-only wrapper is appropriate for a parent route.e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/route.tsx (1)
3-9: LGTM: route shell and path are correctNon-nested parent with Outlet is wired correctly to “/params-ps/non-nested/$foo_”.
e2e/react-router/basic-file-based/tests/params.spec.ts (1)
77-85: Prefer textContent() and inline parse to reduce layout dependencySwitch to textContent and inline JSON.parse for brevity and stability.
File: e2e/react-router/basic-file-based/tests/params.spec.ts (lines 77-85)
- const fooParamsValue = page.getByTestId('foo-params-value') - const fooParamsText = await fooParamsValue.innerText() - const fooParamsObj = JSON.parse(fooParamsText) - expect(fooParamsObj).toEqual({ foo: 'foo' }) + const fooParamsText = await page.getByTestId('foo-params-value').textContent() + expect(JSON.parse(fooParamsText ?? '{}')).toEqual({ foo: 'foo' }) - - const paramsValue = page.getByTestId('foo-bar-params-value') - const paramsText = await paramsValue.innerText() - const paramsObj = JSON.parse(paramsText) - expect(paramsObj).toEqual({ foo: 'foo', bar: 'bar' }) + const paramsText = await page.getByTestId('foo-bar-params-value').textContent() + expect(JSON.parse(paramsText ?? '{}')).toEqual({ foo: 'foo', bar: 'bar' })Quick scan for literal waitForURL globs returned no matches — verify there are no other instances.
e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/$bar.tsx (1)
7-19: LGTM: params accessors used correctly for SoliduseParams accessor and Route.useParams signals are rendered as JSON as expected.
e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/$bar.tsx (1)
7-19: LGTM: React params usage aligns with APIReact version correctly renders plain objects from useParams/Route.useParams.
e2e/solid-router/basic-file-based/src/routes/params-ps/index.tsx (1)
7-33: LGTM: index wiring and test IDs match e2eLinks and headings align with tests; Solid’s class usage is correct.
e2e/react-router/basic-file-based/src/routeTree.gen.ts (6)
41-41: LGTM: imports for the new non-nested params subtree are correct.Paths resolve to expected route modules under params-ps/non-nested.
Also applies to: 65-65, 73-73
225-229: LGTM: route wiring preserves $foo_ semantics.
- Parent/child relationships are correct.
- id retains
$foo_while path/fullPath use$fooas expected.Also applies to: 354-359, 399-403
451-451: Type maps updated consistently (fullPath vs id).
- fullPath/To entries use
$foo; Ids use$foo_. No collisions evident.Please run the new E2E to confirm useParams({ from: '/params-ps/non-nested/$foo_' }) returns foo as expected across navigations.
Also applies to: 467-467, 492-492, 514-514, 529-529, 554-554, 580-580, 599-599, 624-624, 650-650, 666-666, 691-691, 713-713, 728-728, 753-753, 778-778, 797-797, 822-822
1071-1077: LGTM: module augmentation entries match id/path/fullPath expectations.preLoaderRoute and parentRoute pairings look correct.
Also applies to: 1239-1245, 1295-1301
1397-1424: LGTM: WithChildren wrappers for the non-nested subtree.Children collections correctly expose
$barunder$foo_.
848-848: LGTM: root children exposure.ParamsPsNonNestedRouteRoute is surfaced in both the type and runtime maps.
Also applies to: 1563-1564
e2e/solid-router/basic-file-based/src/routeTree.gen.ts (6)
28-28: LGTM: new imports for params-ps index and non-nested subtree.Solid parity with React is clear.
Also applies to: 39-39, 56-56, 64-64
147-151: LGTM: added routes and hierarchy for /params-ps and non-nested foo/bar.id/path alignment mirrors the underscore semantics.
Also applies to: 204-208, 294-299, 339-343
390-400: Type surfaces expanded correctly.
- Full paths include
$foovariants; Ids include$foo_variants only where appropriate.Please run the Solid E2E for the new routes to validate useParams from ‘from’ strings containing
$foo_.Also applies to: 405-406, 423-424, 444-445, 452-453, 458-459, 476-477, 501-502, 513-514, 519-520, 537-538, 562-563, 571-572, 578-579, 595-596, 616-617, 625-626, 631-632, 648-649, 672-673, 685-686, 691-692, 709-710
857-863: LGTM: module augmentation entries for Solid.id/path/fullPath/preLoaderRoute/parentRoute all line up.
Also applies to: 934-940, 1053-1059, 1109-1115
1211-1238: LGTM: WithChildren wrappers for non-nested subtree.Child exposure matches expected structure.
733-734: LGTM: RootRouteChildren now exposes both index and non-nested groups.React/Solid parity maintained.
Also applies to: 739-740
…k#5135) This PR resolves TanStack#5133 by making the _strictParams more conscious of path defining characters and other wildcards. It also adds e2e tests for react and solid <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added “Non-nested path params” pages and links in React and Solid basic-file-based e2e tests at /params-ps/non-nested, plus nested dynamic routes to view single and combined params; UI links and outlets included. - **Bug Fixes** - Improved internal path-parameter resolution so strict param extraction uses the full route path for more accurate matching. - **Tests** - Added end-to-end tests verifying navigation to /params-ps/non-nested/foo/bar and correct partial vs. full param values. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 participant
This PR resolves #5133 by making the _strictParams more conscious of path defining characters and other wildcards. It also adds e2e tests for react and solid
Summary by CodeRabbit
New Features
Bug Fixes
Tests