fix(router-core): support parsed params in matchRoute#7776
fix(router-core): support parsed params in matchRoute#7776LadyBluenotes wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesTyped route matching
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant useMatchRoute
participant RouterCore.matchRoute
participant RouteParser
useMatchRoute->>RouterCore.matchRoute: Match route with typed params
RouterCore.matchRoute->>RouteParser: Parse raw URL parameters
RouteParser-->>RouterCore.matchRoute: Return parsed params or rejection
RouterCore.matchRoute-->>useMatchRoute: Return typed match or false
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit 22af4b8
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview1 package(s) bumped directly, 22 bumped as dependents. 🟩 Patch bumps
|
…ck/router into match-route-parsed-params
Bundle Size Benchmarks
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
Merging this PR will degrade performance by 31.13%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem server server-fn-churn (solid) |
274 KB | 3,551.7 KB | -92.29% |
| ❌ | Memory | mem server serialization-payload (vue) |
3.5 MB | 6.4 MB | -45.95% |
| ❌ | Memory | mem server error-paths not-found (solid) |
427 KB | 696.4 KB | -38.69% |
| ❌ | Memory | mem server error-paths not-found (vue) |
387.9 KB | 467 KB | -16.93% |
| ❌ | Memory | mem server peak-large-page (vue) |
715.4 KB | 760.8 KB | -5.97% |
| ⚡ | Memory | mem server error-paths unmatched (react) |
606.9 KB | 313.3 KB | +93.68% |
| ⚡ | Memory | mem server streaming-peak chunked (vue) |
13.9 MB | 11.1 MB | +25.42% |
| ⚡ | Memory | mem server server-fn-churn (react) |
283 KB | 271.3 KB | +4.31% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing match-route-parsed-params (f396cca) with main (a3e24c3)
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nx Cloud is proposing a fix for your failed CI:
We extracted the anonymous component arrow function into a named InvoiceComponent function to fix the react-hooks/rules-of-hooks ESLint error. The rule requires React component functions to start with an uppercase letter, which the lowercase component property name did not satisfy. This change preserves the full test intent while making the hook usage compliant.
Warning
❌ We could not verify this fix.
Suggested Fix changes
diff --git a/packages/react-router/tests/Matches.test.tsx b/packages/react-router/tests/Matches.test.tsx
index e756a567..996c9d03 100644
--- a/packages/react-router/tests/Matches.test.tsx
+++ b/packages/react-router/tests/Matches.test.tsx
@@ -147,6 +147,16 @@ test('should show pendingComponent of root route', async () => {
test('useMatchRoute matches typed params from routes with custom parse and stringify functions', async () => {
const rootRoute = createRootRoute()
+
+ function InvoiceComponent() {
+ const matchRoute = useMatchRoute()
+ const match = matchRoute({
+ to: '/invoices/$invoiceId',
+ params: { invoiceId: 123 },
+ })
+ return <div data-testid="match">{JSON.stringify(match)}</div>
+ }
+
const invoiceRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/invoices/$invoiceId',
@@ -158,15 +168,7 @@ test('useMatchRoute matches typed params from routes with custom parse and strin
invoiceId: String(invoiceId),
}),
},
- component: () => {
- const matchRoute = useMatchRoute()
- const match = matchRoute({
- to: '/invoices/$invoiceId',
- params: { invoiceId: 123 },
- })
-
- return <div data-testid="match">{JSON.stringify(match)}</div>
- },
+ component: InvoiceComponent,
})
const router = createRouter({
routeTree: rootRoute.addChildren([invoiceRoute]),
🔔 Heads up, your workspace has pending recommendations ↗ to auto-apply fixes for similar failures.
Or Apply changes locally with:
npx nx-cloud apply-locally 9YYT-RTGB
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
Sheraff
left a comment
Sheraff
left a comment
There was a problem hiding this comment.
I think it makes sense to fix this. But also router.matchRoute is broken more generally because it does not take the route tree into account. It could return true for the specific route you're asking for, but when actually compared against the entire tree, another route would match because it had higher matching priority.
| try { | ||
| for (const matchedRoute of this.getRouteBranch(destinationRoute)) { | ||
| const parseParams = | ||
| matchedRoute.options.params?.parse ?? | ||
| matchedRoute.options.parseParams | ||
| if (parseParams) { | ||
| const parsedParams = parseParams(params as Record<string, string>) | ||
| if (parsedParams === false) { | ||
| return false | ||
| } | ||
| Object.assign(params, parsedParams) | ||
| } | ||
| } | ||
| } catch { | ||
| return false | ||
| } |
There was a problem hiding this comment.
I think this could be shortened using the extractStrictParams method that exists in this file
for (const matchedRoutes of this.getRouteBranch(destinationRoute)) {
try {
extractStrictParams(route, params)
} catch {
return false
}
}2 participants
matchRoutenow applies route parameter parsers before it compares or returns matched params. This restores matching for routes that use custom path parameter parsing, including typed params and parser-rejected values.Adds regression coverage in
router-coreand the React, Solid, and Vue Matches tests.Includes the reproducer from #2460.
Closes #2450.
Summary by CodeRabbit
Bug Fixes
Tests