fix(router-core): fix search middleware type to use schema output#7381
Conversation
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. |
|
Need the big picture first? Review this PR in Change Stack to see what changed before going file by file. 📝 WalkthroughWalkthroughThe search middleware generic in ChangesSearch Middleware Type Update
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 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 |
|
Long time TanStack Router fan, first time contributor. I spent a bit of time validating my theory with this one, so hopefully haven't jumped to the wrong conclusion. As an aside, I would love to see more comprehensive support of asymmetrical schemas particularly when it comes to search params. An example use case: I'm unhappy with the appearance of how arrays of strings are encoded in the url by default, and would prefer to have them as space-separated values - being able to do this with Zod codecs would be wonderful (I am already, but it relies on own middleware). |
|
In fact if maintainers agree with conclusions here then I think |
|
@chorobin wondering if you'd have a chance to take a look at this. Not sure what the situation is now with external contributors, but it would be great to have this types issue fixed in some form or another. Thank you! |
|
can you please add a type test? |
|
View your CI Pipeline Execution ↗ for commit 84e83ce
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud is proposing a fix for your failed CI:
We changed stripSearchParams's TValues type from Partial<PickOptional<TSearchSchema>> to Partial<TSearchSchema> so that default values can be provided for required fields — which is the case when a Zod schema uses .default(), making the field required in the output type. This unblocks stripSearchParams({ foo: 'default' }) usage after the middleware context was correctly changed to use the schema output type instead of the input type. The outdated comment noting this as a known limitation has also been removed from the affected test files.
Tip
✅ We verified this fix by re-running @tanstack/vue-router:test:types, @tanstack/react-router:test:unit, @tanstack/solid-router:test:unit and 1 more.
Suggested Fix changes
diff --git a/packages/react-router/tests/link.test.tsx b/packages/react-router/tests/link.test.tsx
index f06a3a8d..ec74e6f1 100644
--- a/packages/react-router/tests/link.test.tsx
+++ b/packages/react-router/tests/link.test.tsx
@@ -5891,7 +5891,6 @@ describe('search middleware', () => {
}),
search: {
middlewares: [
- // this means we cannot get the correct input type for this schema
stripSearchParams({ foo: 'default' }),
retainSearchParams(true),
],
diff --git a/packages/router-core/src/searchMiddleware.ts b/packages/router-core/src/searchMiddleware.ts
index a564650a..b8ac1a73 100644
--- a/packages/router-core/src/searchMiddleware.ts
+++ b/packages/router-core/src/searchMiddleware.ts
@@ -82,9 +82,7 @@ function getValidationDefaultKeys(
export function stripSearchParams<
TSearchSchema,
TOptionalProps = PickOptional<NoInfer<TSearchSchema>>,
- const TValues =
- | Partial<NoInfer<TOptionalProps>>
- | Array<keyof TOptionalProps>,
+ const TValues = Partial<NoInfer<TSearchSchema>> | Array<keyof TOptionalProps>,
const TInput = IsRequiredParams<TSearchSchema> extends never
? TValues | true
: TValues,
diff --git a/packages/solid-router/tests/link.test.tsx b/packages/solid-router/tests/link.test.tsx
index d0fc7ae4..03f71805 100644
--- a/packages/solid-router/tests/link.test.tsx
+++ b/packages/solid-router/tests/link.test.tsx
@@ -5762,7 +5762,6 @@ describe('search middleware', () => {
}),
search: {
middlewares: [
- // this means we cannot get the correct input type for this schema
stripSearchParams({ foo: 'default' }),
retainSearchParams(true),
],
diff --git a/packages/vue-router/tests/link.test.tsx b/packages/vue-router/tests/link.test.tsx
index e047fe59..ed223135 100644
--- a/packages/vue-router/tests/link.test.tsx
+++ b/packages/vue-router/tests/link.test.tsx
@@ -5933,7 +5933,6 @@ describe('search middleware', () => {
}),
search: {
middlewares: [
- // this means we cannot get the correct input type for this schema
stripSearchParams({ foo: 'default' }),
retainSearchParams(true),
],
Because this branch comes from a fork, it is not possible for us to apply fixes directly, but you can apply the changes locally using the available options below.
Apply changes locally with:
npx nx-cloud apply-locally ilK4-KXCt
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/router-core/src/searchMiddleware.ts (1)
71-71: 💤 Low valueExisting type cast reduces return type safety.
The
as anycast bypasses TypeScript's type checking on the middleware's return value. Combined with the widenedTValuestype that now accepts required properties (line 48), there's no compile-time protection against returning a result that's missing required properties.While this is pre-existing code, consider whether the return type could be tightened to
TSearchSchemaor a conditional type that reflects which properties might be stripped. This would provide stronger compile-time guarantees aligned with the PR's goal of improving type safety.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router-core/src/searchMiddleware.ts` at line 71, The `as any` cast on the returned `result` weakens type safety; replace it with a precise type such as `TSearchSchema` (or a conditional type that reflects stripped/optional properties) and adjust the middleware function's generics so the compiler can verify `result` conforms to that type. Concretely, remove `as any` and cast/annotate `result` to `TSearchSchema` (or a conditional mapping of `TValues` → `TSearchSchema`), and tighten the generic constraints around `TValues`/`TSearchSchema` in the search middleware function so missing required properties are caught at compile time. Ensure the variable named `result` and the generics `TValues` and `TSearchSchema` are updated together so the function signature and return type remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/router-core/src/searchMiddleware.ts`:
- Line 71: The `as any` cast on the returned `result` weakens type safety;
replace it with a precise type such as `TSearchSchema` (or a conditional type
that reflects stripped/optional properties) and adjust the middleware function's
generics so the compiler can verify `result` conforms to that type. Concretely,
remove `as any` and cast/annotate `result` to `TSearchSchema` (or a conditional
mapping of `TValues` → `TSearchSchema`), and tighten the generic constraints
around `TValues`/`TSearchSchema` in the search middleware function so missing
required properties are caught at compile time. Ensure the variable named
`result` and the generics `TValues` and `TSearchSchema` are updated together so
the function signature and return type remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e5affdd-481a-43f7-ac59-1786906de873
📒 Files selected for processing (2)
packages/router-core/src/searchMiddleware.tspackages/router-core/tests/searchMiddleware.test-d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/router-core/tests/searchMiddleware.test-d.ts
|
Thank you @schiller-manuel for pushing this over the line |
2 participants
Summary
The type for
searchpassed to a route's search middleware is currently set to the input of whatever is passed tovalidateSearch. In reality this value is the output of the search validator.In the majority of cases these types are the same, but it becomes a pronounced issue with asymmetric schemas, for example with Zod codecs:
The value that we get in the search middleware is a
BigIntbut it's typed as anumber.Changes
Simple switch from
ResolveFullSearchSchemaInputtoResolveFullSearchSchema.Reproduction
You can find here a small TanStack Start project with a demo of the mismatch: https://github.com/sprusr/tanstack-router-search-middleware-types
Summary by CodeRabbit