-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix(router-core): fix typing of search in remountDeps #5362
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(router-core): fix typing of search in remountDeps #5362
Conversation
WalkthroughUpdated the type of the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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 |
thanks for this PR! can you please add a typetest and unit test for this? |
sure! I added tests |
View your CI Pipeline Execution ↗ for commit 6077beb
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/router-core/tests/remountDeps.test-d.ts (1)
11-11
: Consider a more precise test name for the type-level assertion.The test name "search field should be directly accessible" is somewhat ambiguous for a type-level test. Consider a name like "search field type should match SearchSchema exactly" or "search field should not have nested search key" to better reflect that this is testing compile-time type inference rather than runtime accessibility.
packages/router-core/tests/remountDeps.test.ts (1)
5-5
: Consider distinguishing the test name from the type test.Both this runtime test and the type test in
remountDeps.test-d.ts
use the same name "search field should be directly accessible", but they verify different aspects (runtime vs. compile-time). Consider using a more specific name like "search field value should be directly accessible at runtime" to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router-core/tests/remountDeps.test-d.ts
(1 hunks)packages/router-core/tests/remountDeps.test.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-core/tests/remountDeps.test-d.ts
packages/router-core/tests/remountDeps.test.ts
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/tests/remountDeps.test-d.ts
packages/router-core/tests/remountDeps.test.ts
🔇 Additional comments (1)
packages/router-core/tests/remountDeps.test.ts (1)
10-20
: LGTM! Test validates runtime behavior correctly.The test appropriately verifies that the search field is directly accessible at runtime without extra nesting. The mock structure matches the expected
RemountDepsOptions
type signature, and the assertion confirms the fix addresses the reported issue.
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/nitro-v2-vite-plugin
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@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-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-static-server-functions
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
close #5361
use
ResolveFullSearchSchema
instead ofFullSearchSchemaOption
search
is iffered as correctlySummary by CodeRabbit
Refactor
Tests