fix(router-core): parse _strictParams. #5120
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughIntroduces strict per-route parameter parsing and error handling in router-core, updates match construction to use strictly parsed params, and adds new integration tests for useParams in React and Solid routers validating nested params parsing/stringifying and loader data interactions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant R as Router
participant Rt as Route
participant P as Params Parser
participant LD as Loader
U->>R: Navigate to URL
R->>Rt: Match route tree
Note right of R: Interpolate route.id to derive strict params
R->>P: strictParse(params)
alt parse OK
P-->>R: parsedParams
R->>Rt: Build match with _strictParams=parsedParams
R->>LD: Invoke loader with parsedParams
LD-->>R: loaderData
R-->>U: Render with useParams/useLoaderData
else parse throws
P-->>R: error
Note right of R: Create PathParamError<br/>(record or throw)
alt throwOnError
R-->>U: Throw error
else record error
R->>Rt: Build match with paramsError
R-->>U: Render (error available on match)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
View your CI Pipeline Execution ↗ for commit 2b966b1
☁️ 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/solid-router/tests/useParams.test.tsx (2)
181-191: Avoid wrapping router.load() and clicks in waitFor (can cause repeated side effects).Call router.load() once and click directly; wait on subsequent UI assertions instead.
- await waitFor(() => router.load()) + await router.load() ... - await waitFor(() => fireEvent.click(firstCategoryLink)) + fireEvent.click(firstCategoryLink) ... - await waitFor(() => fireEvent.click(allCategoryLink)) + fireEvent.click(allCategoryLink) ... - await waitFor(() => fireEvent.click(secondPostLink)) + fireEvent.click(secondPostLink)Also applies to: 225-235
210-214: Guard against null textContent and specify radix in parseInt.Prevents TS nullability complaints and ensures base-10 parsing.
- let renderedPost = { - id: parseInt(postIdValue.textContent), + let renderedPost = { + id: parseInt(postIdValue.textContent!, 10), title: postTitleValue.textContent, category: postCategory.textContent, } ... - renderedPost = { - id: parseInt(postIdValue.textContent), + renderedPost = { + id: parseInt(postIdValue.textContent!, 10), title: postTitleValue.textContent, category: postCategory.textContent, }Also applies to: 246-251
packages/router-core/src/router.ts (2)
1213-1218: Parse-cache missed on fullPath interpolation.interpolatePath supports parseCache; passing it avoids re-parsing templates across navigations.
- const { interpolatedPath } = interpolatePath({ + const { interpolatedPath } = interpolatePath({ path: route.fullPath, params: routeParams, decodeCharMap: this.pathParamsDecodeCharMap, + parseCache: this.parsePathnameCache, })
1227-1250: Strict per-route param parsing looks good; minor robustness tweak optional.If a parse function returns undefined/null, Object.assign will no-op but reads cleaner guarding the result.
- try { - Object.assign(strictParams, strictParseParams(strictParams as any)) + try { + const parsed = strictParseParams(strictParams as any) + if (parsed) Object.assign(strictParams, parsed)packages/react-router/tests/useParams.test.tsx (2)
182-199: Simplify act usage and avoid wrapping fireEvent with act.react-testing-library already wraps fireEvent in act. Use a single awaited act for router.load and click directly for events.
- await act(() => router.load()) + await act(async () => { await router.load() }) ... - await act(() => fireEvent.click(firstCategoryLink)) + fireEvent.click(firstCategoryLink) ... - await act(() => fireEvent.click(all-category-link)) + fireEvent.click(allCategoryLink) ... - await act(() => fireEvent.click(secondPostLink)) + fireEvent.click(secondPostLink)Also applies to: 226-236
210-215: Specify radix and guard null when parsing textContent.Avoids subtle parse issues and TS nullability warnings.
- let renderedPost = { - id: parseInt(postIdValue.textContent), + let renderedPost = { + id: parseInt(postIdValue.textContent!, 10), title: postTitleValue.textContent, category: postCategory.textContent, } ... - renderedPost = { - id: parseInt(postIdValue.textContent), + renderedPost = { + id: parseInt(postIdValue.textContent!, 10), title: postTitleValue.textContent, category: postCategory.textContent, }Also applies to: 247-252
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/react-router/tests/useParams.test.tsx(1 hunks)packages/router-core/src/router.ts(4 hunks)packages/solid-router/tests/useParams.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/router-core/src/router.ts (1)
packages/router-core/src/path.ts (1)
interpolatePath(371-485)
⏰ 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 (4)
packages/solid-router/tests/useParams.test.tsx (1)
138-149: Using useParams({ from: postRoute.fullPath }) here is correct and exercises strict parsing.This ensures leaf-route scoped parsed params are read. Nice.
packages/router-core/src/router.ts (2)
1219-1226: Verify matchId stability with route.id-based interpolation.Switching matchId to interpolatePath(route.id, { leaveWildcards: true }) changes the identity key. Please confirm no collisions for routes with optional/wildcard segments (e.g., siblings differing only by optional params) and that cache hits remain correct across param changes.
Would you run a quick check in your suite to ensure identical URLs across reloads resolve to the same match.id and that toggling optional params yields distinct match ids where expected?
Also applies to: 1258-1259
1274-1282: Correctly plumbs _strictParams and paramsError into matches.This aligns the match object with strict parsing semantics and per-route error surfacing.
Also applies to: 1299-1311
packages/react-router/tests/useParams.test.tsx (1)
139-170: LGTM: useParams is scoped to postRoute ensuring parsed values are asserted.Good coverage of category mapping and numeric id parsing, consistent with stringify handlers.
This was referenced Sep 11, 2025
LadyBluenotes
pushed a commit
to LadyBluenotes/router
that referenced
this pull request
Sep 19, 2025
with changing useParams to make use of _strictParams it became apparent that _strictParams did not apply param parsing correctly. This PR resolves that problem and hence useParams now return the parsed params correctly. This PR also adds unit tests in both react-router and solid-router to test this expected result. A separate PR has been created to merge into alpha (TanStack#5121) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - New Features - Stricter route parameter parsing and validation with support for custom parse/stringify. - Clearer per-route parameter error reporting, with optional throw-on-error behavior. - More consistent parameter values across nested routes and loaders. - Tests - Added integration tests validating useParams with nested routing, custom param parsing/stringifying, and loader data in both React and Solid routers. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
with changing useParams to make use of _strictParams it became apparent that _strictParams did not apply param parsing correctly. This PR resolves that problem and hence useParams now return the parsed params correctly.
This PR also adds unit tests in both react-router and solid-router to test this expected result.
A separate PR has been created to merge into alpha (#5121)
Summary by CodeRabbit
New Features
Tests