fix: parse params union inference#7306
Conversation
📝 WalkthroughWalkthroughType inference improvements for the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
|
View your CI Pipeline Execution ↗ for commit 7de1159
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview1 package(s) bumped directly, 22 bumped as dependents. 🟩 Patch bumps
|
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-router/tests/fileRoute.test-d.tsx (1)
155-162: ⚡ Quick winAdd a matching negative test for deprecated
parseParamstoo.
route.tsnow enforces the same validation onparseParams, but this file currently guards onlyparams.parse.💡 Proposed test addition
test('when file route params.parse drops a path param', () => { createFileRoute('/invoices/$invoiceId')({ params: { // `@ts-expect-error` parsed params must keep path param keys parse: () => ({}), }, }) }) + +test('when deprecated parseParams drops a path param', () => { + createFileRoute('/invoices/$invoiceId')({ + // `@ts-expect-error` parsed params must keep path param keys + parseParams: () => ({}), + }) +})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-router/tests/fileRoute.test-d.tsx` around lines 155 - 162, Add a matching negative test that mirrors the existing "when file route params.parse drops a path param" case but targets the deprecated params.parseParams API: call createFileRoute('/invoices/$invoiceId') with params: { /* `@ts-expect-error` parsed params must keep path param keys */ parseParams: () => ({}) } and assert TypeScript emits the expected error; reference the same test harness and test name pattern (e.g., "when file route params.parseParams drops a path param") and reuse createFileRoute to locate where to add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-router/tests/fileRoute.test-d.tsx`:
- Around line 155-162: Add a matching negative test that mirrors the existing
"when file route params.parse drops a path param" case but targets the
deprecated params.parseParams API: call createFileRoute('/invoices/$invoiceId')
with params: { /* `@ts-expect-error` parsed params must keep path param keys */
parseParams: () => ({}) } and assert TypeScript emits the expected error;
reference the same test harness and test name pattern (e.g., "when file route
params.parseParams drops a path param") and reuse createFileRoute to locate
where to add the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85d23201-6b08-4a21-9225-fa8d6a60bf7c
📒 Files selected for processing (3)
.changeset/fix-parse-params-union.mdpackages/react-router/tests/fileRoute.test-d.tsxpackages/router-core/src/route.ts
Merging this PR will not alter performance
Comparing Footnotes
|
Summary by CodeRabbit
params.parsewith discriminated-union path parameters to properly resolve the expected type shape.