feat(tanstack): return query key and pass in abort signal to fetcher #1903
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the Tanstack Query library across various frameworks (React, Vue, Svelte). Key changes include the restructuring of query hooks to enhance the handling of Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (16)
packages/plugins/tanstack-query/src/runtime-v5/vue.ts (4)
74-77: Correctness ofoptimisticUpdateProperty CheckConsider simplifying the check for
optimisticUpdateingetQueryKey. Currently, it usesoptionsValue?.optimisticUpdate !== false, which may lead to unintended behavior ifoptimisticUpdateisundefined. Explicitly handleundefinedto ensure correct query key generation.Apply this diff to clarify the intention:
const queryKey = getQueryKey(model, url, args, { infinite: false, - optimisticUpdate: optionsValue?.optimisticUpdate !== false, + optimisticUpdate: optionsValue?.optimisticUpdate === true, });
81-86: Type Inconsistency inqueryFnParametersThe
queryFndestructuresqueryKeybut may not need it sincequeryKeyis already available in the outer scope. This could lead to confusion or unintended usage.Consider refactoring the
queryFnto avoid shadowing the outerqueryKey:queryFn: ({ signal }) => { - const [_prefix, _model, _op, args] = queryKey; + const argsValue = toValue(args); const reqUrl = makeUrl(url, argsValue); return fetcher<TQueryFnData, false>(reqUrl, { signal }, fetch, false); },
121-126: Ensure Consistent Access toargsinqueryFnWithin
queryFn,argsis obtained fromqueryKey, but it might be clearer and safer to use theargsparameter directly, especially sincequeryKeymay have a complex structure.Consider refactoring to access
argsdirectly:queryFn: ({ queryKey, pageParam, signal }) => { - const [_prefix, _model, _op, args] = queryKey; - const reqUrl = makeUrl(url, pageParam ?? toValue(args)); + const argsValue = pageParam ?? toValue(args); + const reqUrl = makeUrl(url, argsValue); return fetcher<TQueryFnData, false>(reqUrl, { signal }, fetch, false); },
Line range hint
150-161: Investigate Typing Issue inuseModelMutationThe comment
// TODO: figure out the typing problemindicates there is a known typing issue withfinalOptions. This should be addressed to ensure type safety and maintainability.Would you like assistance in resolving the typing issue with
finalOptions? I can help refactor the code to properly type this variable.packages/plugins/tanstack-query/src/runtime/react.ts (3)
67-70: Consistent Use ofoptionsingetQueryKeyIn the
useModelQueryfunction, consider passingoptions?.optimisticUpdatedirectly togetQueryKeyfor clarity.Apply this diff to simplify the code:
const queryKey = getQueryKey(model, url, args, { infinite: false, - optimisticUpdate: options?.optimisticUpdate !== false, + optimisticUpdate: options?.optimisticUpdate, });
73-77: SimplifyqueryFninuseQueryThe
queryFncurrently constructsreqUrloutside of the function passed touseQuery. This can be simplified for better readability.Apply this diff:
return { queryKey, ...useQuery<TQueryFnData, TError, TData>({ queryKey, - queryFn: ({ signal }) => fetcher<TQueryFnData, false>(reqUrl, { signal }, fetch, false), + queryFn: ({ signal }) => fetcher<TQueryFnData, false>(makeUrl(url, args), { signal }, fetch, false), ...options, }), };
98-108: Reuse Code BetweenuseModelQueryanduseInfiniteModelQueryConsider abstracting the common logic between
useModelQueryanduseInfiniteModelQueryto reduce code duplication and enhance maintainability.packages/plugins/tanstack-query/src/runtime/svelte.ts (4)
68-71: Handleoptions.optimisticUpdateUndefined CaseWhen accessing
options?.optimisticUpdate, ensure that the behavior is well-defined ifoptionsis undefined.Apply this diff to provide a default value:
const queryKey = getQueryKey(model, url, args, { infinite: false, - optimisticUpdate: options?.optimisticUpdate !== false, + optimisticUpdate: options?.optimisticUpdate !== false ?? true, });
76-80: Type Inference forqueryFnThe explicit typing of
queryFnmight be unnecessary if TypeScript can infer the type. Removing explicit types can simplify the code.Apply this diff:
-const queryFn: QueryFunction<TQueryFnData, QueryKey> = ({ signal }) => +const queryFn = ({ signal }) => fetcher<TQueryFnData, false>(reqUrl, { signal }, fetch, false);
97-102: Potential Unnecessary Derived StoreThe use of
derivedto returnqueryKeyalongside the result might add unnecessary complexity. Evaluate if directly returning an object is sufficient.
Line range hint
124-125: Typo in Function Signature CommentThe function description mentions creating a "POST mutation," but the function is for creating an infinite query.
Correct the comment to accurately reflect the function's purpose.
-/** - * Creates a POST mutation with svelte-query. +/** + * Creates a svelte-query infinite query.packages/plugins/tanstack-query/src/runtime-v5/svelte.ts (2)
2-2: Unused Import:QueryKeyThe imported
QueryKeyfrom@tanstack/react-query-v5is not used in this file. Consider removing it to clean up the imports.Apply this diff:
-import { QueryKey } from '@tanstack/react-query-v5';
17-17: Consistent Ordering of ImportsThe import order combines external libraries and local modules. Consider grouping imports by their origin for better readability.
Group imports from external libraries together.
packages/plugins/tanstack-query/src/runtime/vue.ts (3)
71-77: Handle UndefinedoptionsValueSimilar to changes in the
runtime-v5/vue.tsfile, ensure thatoptionsValueis safely handled in case it's undefined.Consider providing a default value or handling the undefined case explicitly.
81-86: SimplifyqueryFnLogicAvoid destructuring
queryKeywithinqueryFnif not necessary, as it may cause confusion.Refactor
queryFnto useargsdirectly:queryFn: ({ signal }) => { - const [_prefix, _model, _op, args] = queryKey; const reqUrl = makeUrl(url, toValue(args)); return fetcher<TQueryFnData, false>(reqUrl, { signal }, fetch, false); },
Line range hint
150-161: Address Typing Problem inuseModelMutationThe comment indicates a typing issue that needs resolution to ensure the code is type-safe.
Would you like assistance in resolving the typing issue mentioned in the comment? I can help adjust the types for
finalOptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
packages/plugins/tanstack-query/src/runtime-v5/react.ts(4 hunks)packages/plugins/tanstack-query/src/runtime-v5/svelte.ts(6 hunks)packages/plugins/tanstack-query/src/runtime-v5/vue.ts(2 hunks)packages/plugins/tanstack-query/src/runtime/react.ts(2 hunks)packages/plugins/tanstack-query/src/runtime/svelte.ts(3 hunks)packages/plugins/tanstack-query/src/runtime/vue.ts(2 hunks)packages/plugins/tanstack-query/tests/plugin.test.ts(4 hunks)
🔇 Additional comments (12)
packages/plugins/tanstack-query/src/runtime-v5/vue.ts (2)
131-131: 🛠️ Refactor suggestion
Avoid Using any Type for queryOptions
Using any undermines type safety. It's better to let TypeScript infer the correct type or specify it explicitly.
Apply this diff to remove the any type annotation:
-const queryOptions: any = computed<
+const queryOptions = computed<
Omit<UseInfiniteQueryOptions<TQueryFnData, TError, InfiniteData<TData>>, 'queryKey' | 'initialPageParam'>
>(() => ({Likely invalid or redundant comment.
71-73: Ensure optionsValue Handles Undefined Appropriately
The extraction of optionsValue using toValue may result in undefined if options is not provided. Ensure that subsequent usage of optionsValue safely handles the possibility of undefined.
Verify that optionsValue is appropriately handled:
✅ Verification successful
optionsValue is Safely Handled Throughout the Code
Based on the code analysis, optionsValue is used safely throughout the implementation:
- When accessing
optimisticUpdate: Uses optional chainingoptionsValue?.optimisticUpdate !== false - When spreading into query options: Spread operator safely handles undefined
...optionsValue - All usages employ proper null-safe patterns:
- Optional chaining for property access
- Default values through spread operator
- No direct property access without checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if `optionsValue` is used safely when undefined.
# Search for usages of `optionsValue` where it's accessed without a null check.
rg --pcre2 "optionsValue\.\w+" -A2 packages/plugins/tanstack-query/src/
Length of output: 73
Script:
#!/bin/bash
# Let's check the complete context of how optionsValue is used
rg "optionsValue" -B2 -A5 packages/plugins/tanstack-query/src/
Length of output: 6613
Script:
#!/bin/bash
# Let's also look at the complete function implementation to understand the context better
ast-grep --pattern 'const optionsValue = toValue<$_>($_)'
Length of output: 1373
Script:
#!/bin/bash
# Let's check the complete file content to understand the full context
cat packages/plugins/tanstack-query/src/runtime-v5/vue.ts
Length of output: 7802
packages/plugins/tanstack-query/src/runtime-v5/svelte.ts (2)
124-125: Ensure Correct Generic Parameters in QueryFunction
The QueryFunction type might require specific generic parameters. Verify that the usage aligns with the type definition in @tanstack/svelte-query-v5.
Check the QueryFunction type definition for correct usage.
76-77: Incorrect Import Path for QueryFunction
Ensure that QueryFunction is imported from the correct package. Currently, it's imported from @tanstack/svelte-query-v5, but you might need to verify if this is the intended source.
Run the following script to check the available exports:
packages/plugins/tanstack-query/src/runtime-v5/react.ts (4)
103-114: LGTM! Consistent implementation with useModelQuery.
The suspense version correctly mirrors the non-suspense implementation, maintaining consistency in both queryKey return and abort signal support.
134-145: LGTM! Infinite query implementation properly adapted.
The infinite query version correctly implements:
- queryKey return for infinite queries
- Signal support while maintaining pageParam functionality
168-179: LGTM! Consistent implementation with useInfiniteModelQuery.
The suspense infinite query version correctly mirrors the non-suspense implementation.
71-82: LGTM! Implementation aligns with TanStack Query v5 patterns.
The changes correctly implement both requirements:
- Return queryKey alongside results for better query management
- Pass abort signal to fetcher for request cancellation support
Let's verify the queryKey structure matches usage in tests:
✅ Verification successful
Implementation correctly returns queryKey and follows TanStack Query v5 patterns
The changes have been verified:
- The queryKey is correctly generated using the
getQueryKeyfunction with proper parameters (model, url, args, options) - The queryKey is returned alongside the query results in a spread object
- The abort signal is properly passed to the fetcher function
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify queryKey structure consistency across codebase
# Expected: queryKey structure should match the test usage
# Check queryKey usage in tests
rg -A 2 'queryKey.*=.*use.*Query'
Length of output: 35
Script:
#!/bin/bash
# Let's try a broader search for queryKey usage in test files
fd "test|spec" -e ts -e tsx --exec rg -l "queryKey"
# Then let's look for actual test implementations using useModelQuery
rg "useModelQuery" -A 5 -B 5
# Also check for any test files specifically for this feature
fd "tanstack|query" -e test.ts -e test.tsx -e spec.ts -e spec.tsx
Length of output: 74432
Script:
#!/bin/bash
# Let's check the test file for queryKey usage and structure
rg -A 5 "queryKey.*=.*getQueryKey" packages/plugins/tanstack-query/tests/react-hooks-v5.test.tsx
# Also check the getQueryKey implementation
rg -A 10 "export.*getQueryKey" packages/plugins/tanstack-query/src/runtime/common.ts
Length of output: 542
packages/plugins/tanstack-query/tests/plugin.test.ts (4)
57-58: LGTM! Tests properly verify queryKey return.
Tests correctly verify the new queryKey functionality for both regular and infinite queries.
Also applies to: 64-67
129-132: LGTM! Suspense tests properly verify queryKey return.
Tests correctly verify the queryKey functionality for suspense infinite queries.
149-150: LGTM! Vue tests properly verify queryKey return.
Tests correctly verify the queryKey functionality while properly handling Vue's reactivity system.
Also applies to: 156-159
225-226: LGTM! Svelte tests properly verify queryKey return.
Tests correctly verify the queryKey functionality while properly handling Svelte's store pattern using get().
Also applies to: 232-235
| ) { | ||
| // CHECKME: vue-query's `useInfiniteQuery`'s input typing seems wrong | ||
| const queryOptions: any = computed(() => ({ | ||
| queryKey: getQueryKey(model, url, args, { infinite: true, optimisticUpdate: false }), | ||
| queryFn: ({ queryKey, pageParam }: { queryKey: QueryKey; pageParam?: unknown }) => { | ||
| const queryKey = getQueryKey(model, url, args, { infinite: true, optimisticUpdate: false }); | ||
| const queryOptions: any = computed< |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Typings for queryOptions May Be Too Broad
The queryOptions variable is typed as any, which can mask potential type errors and reduce the benefits of TypeScript's static typing.
Apply this diff to provide a more specific type:
-const queryOptions: any = computed<
+const queryOptions = computed<
Omit<UseInfiniteQueryOptions<TQueryFnData, TError, InfiniteData<TData>>, 'queryKey' | 'initialPageParam'>
>(() => ({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ) { | |
| // CHECKME: vue-query's `useInfiniteQuery`'s input typing seems wrong | |
| const queryOptions: any = computed(() => ({ | |
| queryKey: getQueryKey(model, url, args, { infinite: true, optimisticUpdate: false }), | |
| queryFn: ({ queryKey, pageParam }: { queryKey: QueryKey; pageParam?: unknown }) => { | |
| const queryKey = getQueryKey(model, url, args, { infinite: true, optimisticUpdate: false }); | |
| const queryOptions: any = computed< | |
| ) { | |
| // CHECKME: vue-query's `useInfiniteQuery`'s input typing seems wrong | |
| const queryKey = getQueryKey(model, url, args, { infinite: true, optimisticUpdate: false }); | |
| const queryOptions = computed< |
| const queryKey = getQueryKey(model, url, args, { infinite: true, optimisticUpdate: false }); | ||
| const queryOptions: any = computed< | ||
| Omit<UseInfiniteQueryOptions<TQueryFnData, TError, InfiniteData<TData>>, 'queryKey' | 'initialPageParam'> | ||
| >(() => ({ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Typings for queryOptions May Be Too Broad
Using any for queryOptions reduces type safety.
Remove any and let TypeScript infer the type:
-const queryOptions: any = computed<
+const queryOptions = computed<
Omit<UseInfiniteQueryOptions<TQueryFnData, TError, InfiniteData<TData>>, 'queryKey' | 'initialPageParam'>
>(() => ({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const queryKey = getQueryKey(model, url, args, { infinite: true, optimisticUpdate: false }); | |
| const queryOptions: any = computed< | |
| Omit<UseInfiniteQueryOptions<TQueryFnData, TError, InfiniteData<TData>>, 'queryKey' | 'initialPageParam'> | |
| >(() => ({ | |
| const queryKey = getQueryKey(model, url, args, { infinite: true, optimisticUpdate: false }); | |
| const queryOptions = computed< | |
| Omit<UseInfiniteQueryOptions<TQueryFnData, TError, InfiniteData<TData>>, 'queryKey' | 'initialPageParam'> | |
| >(() => ({ |
fixes #1872