fix(vue-query): preserve TQueryKey inference with generic params (#8199)#10584
Open
ousamabenyounes wants to merge 2 commits intoTanStack:mainfrom
Open
fix(vue-query): preserve TQueryKey inference with generic params (#8199)#10584ousamabenyounes wants to merge 2 commits intoTanStack:mainfrom
ousamabenyounes wants to merge 2 commits intoTanStack:mainfrom
Conversation
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughModifies vue-query types and adds a type test and changeset: ChangesPreserve TQueryKey inference
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 2046279
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/vue-query/src/__tests__/useQuery.test-d.ts (1)
155-174: Strengthen the regression assertion.
assertType(useBasket('fruit'))only verifies the expression compiles. The original bug per#8199was thatTQueryKeyfell back tounknown/QueryKeyand the result was typed asUseQueryReturnType<unknown, Error>— that broader form would also compile here. To lock the regression, assert the propagatedTDataso a future regression tounknownwould fail.♻️ Proposed stronger assertion
- assertType(useBasket('fruit')) + const query = reactive(useBasket('fruit')) + expectTypeOf(query.data).toEqualTypeOf<'apple' | 'broccoli' | undefined>()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vue-query/src/__tests__/useQuery.test-d.ts` around lines 155 - 174, Replace the weak compile-only assertion assertType(useBasket('fruit')) with a concrete type assertion that verifies the propagated TData; e.g. assertType<UseQueryReturnType<'apple', Error>>(useBasket('fruit')). Update the test to reference useBasket and getBasket so it explicitly expects the data type 'apple' (not unknown), using the UseQueryReturnType generic to lock the regression.
🤖 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/vue-query/src/__tests__/useQuery.test-d.ts`:
- Around line 155-174: Replace the weak compile-only assertion
assertType(useBasket('fruit')) with a concrete type assertion that verifies the
propagated TData; e.g. assertType<UseQueryReturnType<'apple',
Error>>(useBasket('fruit')). Update the test to reference useBasket and
getBasket so it explicitly expects the data type 'apple' (not unknown), using
the UseQueryReturnType generic to lock the regression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 997560b3-0722-4838-94f9-598c1ceb3889
📒 Files selected for processing (3)
.changeset/vue-query-generic-querykey-inference.mdpackages/vue-query/src/__tests__/useQuery.test-d.tspackages/vue-query/src/useQuery.ts
Contributor
Author
|
Thanks @coderabbitai — addressed the nitpick in beb9b81.
Verified locally:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
(´• ω •`) ♡ 🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
…Stack#8199) The mapped UseQueryOptions type wrapped queryKey in MaybeRefDeep<DeepUnwrapRef<TQueryKey>>. The recursive DeepUnwrapRef on a generic TQueryKey is opaque to TS's homomorphic mapped type inference, so TQueryKey collapsed to its default (QueryKey) when useQuery was called from a composable that propagates a generic into the queryKey. For the queryKey property only, drop DeepUnwrapRef and use a single- level MaybeRef<TQueryKey> wrap. Inference now binds TQueryKey to the supplied queryKey shape; queryFn still receives DeepUnwrapRef<TQueryKey> via the existing mapping for non-queryKey properties. Co-Authored-By: Ora Studio <noreply@oratelecom.net>
…xplicit TData
Per CodeRabbit feedback: bare assertType(useBasket('fruit')) only verified
the call compiled, which would still pass under the old buggy behavior
(TQueryKey collapsing to QueryKey, TData inferred as unknown). Lock the
regression by asserting the propagated TData explicitly.
ousamabenyounes
force-pushed
the
fix/issue-8199
branch
from
beb9b81 to
2046279
Compare
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
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.
Summary
Fixes #8199.
UseQueryOptionswraps every property ofQueryObserverOptionsinMaybeRefDeep<…DeepUnwrapRef<TQueryKey>…>. For thequeryKeyproperty that produced an inference-hostile type — the recursiveDeepUnwrapRefover a genericTQueryKeyis opaque to TypeScript's homomorphic mapped-type inference, soTQueryKeycollapsed to its default (QueryKey) wheneveruseQuerywas called inside a composable that propagated a generic into the key.This PR special-cases the
queryKeyproperty in the mapped type so it is wrapped only in a single-levelMaybeRef<TQueryKey>. Inference now bindsTQueryKeyto the supplied key shape, while every other property keeps the existingMaybeRefDeep<…DeepUnwrapRef<TQueryKey>…>mapping (soqueryFn'squeryKeyparameter is still the unwrapped form).Repro from the issue (now compiles cleanly and types as
UseQueryReturnType<'apple' | 'broccoli', Error>):The same code already worked in
react-query; this alignsvue-querybehavior with it.Verification
useQuery.test-d.tsreproducing the exact code from the issuepnpm exec nx run @tanstack/vue-query:test:types— passes on TS 5.4 → 6.0pnpm test:lib— vue 2 / 2.7 / 3 all 290/290 pass, 0 regressionspnpm exec nx run @tanstack/vue-query:test:eslint— no errorsqueryKey: [...key, refValue](refs as elements) andqueryKey: ref([...])(ref-wrapped key) patterns still typecheckpatchfor@tanstack/vue-query)Generated by Ora Studio
Vibe coded by ousamabenyounes
Summary by CodeRabbit
Bug Fixes
Tests
Chores
Vibe Coded by Ousama Ben Younes
Developed With Ora Studio (Claude Code)