-
Notifications
You must be signed in to change notification settings - Fork 0
Clone feature/imperative infinite queries #19
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -124,24 +124,27 @@ export class InfiniteQueryObserver< | |
| > | ||
| } | ||
|
|
||
| fetchNextPage( | ||
| options?: FetchNextPageOptions, | ||
| ): Promise<InfiniteQueryObserverResult<TData, TError>> { | ||
| fetchNextPage({ pageParam, ...options }: FetchNextPageOptions = {}): Promise< | ||
| InfiniteQueryObserverResult<TData, TError> | ||
| > { | ||
| return this.fetch({ | ||
| ...options, | ||
| meta: { | ||
| fetchMore: { direction: 'forward' }, | ||
| fetchMore: { direction: 'forward', pageParam }, | ||
| }, | ||
|
Comment on lines
+127
to
134
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # First, locate and examine infiniteQueryObserver.ts around the specified lines
fd infiniteQueryObserver.ts --type fRepository: coderabbit-test/query Length of output: 113 🏁 Script executed: # Also find types.ts in the same directory structure
fd types.ts packages/query-core/srcRepository: coderabbit-test/query Length of output: 97 🏁 Script executed: # Once found, let me check the exact content of infiniteQueryObserver.ts at lines 127-148
cat -n packages/query-core/src/infiniteQueryObserver.ts | sed -n '120,150p'Repository: coderabbit-test/query Length of output: 993 🏁 Script executed: # Now check the types.ts file for FetchNextPageOptions and FetchPreviousPageOptions
cat -n packages/query-core/src/types.ts | grep -A 20 "FetchNextPageOptions\|FetchPreviousPageOptions"Repository: coderabbit-test/query Length of output: 2604 🏁 Script executed: # Verify there are no other uses of pageParam in FetchNextPageOptions or related types
rg "pageParam" packages/query-core/src/types.tsRepository: coderabbit-test/query Length of output: 306 🏁 Script executed: # Check if there are any imports or extensions that might add pageParam to these interfaces
rg "FetchNextPageOptions\|FetchPreviousPageOptions" packages/query-core/src/types.ts | head -20Repository: coderabbit-test/query Length of output: 47 🏁 Script executed: # Let me check the full context around where pageParam appears in types.ts
rg -B 5 -A 5 "pageParam" packages/query-core/src/types.ts | head -100Repository: coderabbit-test/query Length of output: 931 🏁 Script executed: # Check if the interfaces have been updated - get more context around lines 586-608
cat -n packages/query-core/src/types.ts | sed -n '580,620p'Repository: coderabbit-test/query Length of output: 1829 🏁 Script executed: # Check if there's a tsconfig.json or build configuration that might suppress these errors
find packages/query-core -name "tsconfig.json" -o -name "tsconfig.*.json" | head -5Repository: coderabbit-test/query Length of output: 178 🏁 Script executed: # Let me verify the actual method signature once more and check if there are any type assertions or other overloads
rg -B 10 -A 10 "fetchNextPage\(" packages/query-core/src/infiniteQueryObserver.ts | head -40Repository: coderabbit-test/query Length of output: 566 🏁 Script executed: # Check what ResultOptions contains
rg -B 5 -A 15 "interface ResultOptions" packages/query-core/src/types.tsRepository: coderabbit-test/query Length of output: 594 🏁 Script executed: # Let me also search for any declaration merging or augmentation of FetchNextPageOptions
rg "FetchNextPageOptions" packages/query-core/src/ -A 5Repository: coderabbit-test/query Length of output: 2018 🏁 Script executed: # Check the entire infiniteQueryObserver.ts file for any type definitions or overloads that might affect this
wc -l packages/query-core/src/infiniteQueryObserver.tsRepository: coderabbit-test/query Length of output: 117 🏁 Script executed: # Check if there are any function overloads or declarations in the file
rg "fetchNextPage|fetchPreviousPage" packages/query-core/src/infiniteQueryObserver.ts -B 3Repository: coderabbit-test/query Length of output: 558 🏁 Script executed: # Let's verify the exact line numbers and see if there are any decorators or type annotations that might bypass the type check
cat -n packages/query-core/src/infiniteQueryObserver.ts | sed -n '120,155p'Repository: coderabbit-test/query Length of output: 1107 🏁 Script executed: # Let's check if the code actually compiles by looking for build errors or CI configuration
cat packages/query-core/tsconfig.json | head -50Repository: coderabbit-test/query Length of output: 285 🏁 Script executed: # Check if there's a way the types could be different - let me search for any conditional types or generics that might make pageParam available
ast-grep --pattern 'export interface FetchNextPageOptions {
$$$
}'Repository: coderabbit-test/query Length of output: 47 🏁 Script executed: # Final verification: does TypeScript actually report an error? Let me check the complete method signature with full context
sed -n '127,136p' packages/query-core/src/infiniteQueryObserver.tsRepository: coderabbit-test/query Length of output: 329 Type mismatch: The methods destructure Add export interface FetchNextPageOptions extends ResultOptions {
/**
* If set to `true`, calling `fetchNextPage` repeatedly will invoke `queryFn` every time,
* whether the previous invocation has resolved or not. Also, the result from previous invocations will be ignored.
*
* If set to `false`, calling `fetchNextPage` repeatedly won't have any effect until the first invocation has resolved.
*
* Defaults to `true`.
*/
cancelRefetch?: boolean
+ pageParam?: unknown
}
export interface FetchPreviousPageOptions extends ResultOptions {
/**
* If set to `true`, calling `fetchPreviousPage` repeatedly will invoke `queryFn` every time,
* whether the previous invocation has resolved or not. Also, the result from previous invocations will be ignored.
*
* If set to `false`, calling `fetchPreviousPage` repeatedly won't have any effect until the first invocation has resolved.
*
* Defaults to `true`.
*/
cancelRefetch?: boolean
+ pageParam?: unknown
}If you need stronger typing, make the interfaces generic (e.g.,
🤖 Prompt for AI Agents |
||
| }) | ||
| } | ||
|
Comment on lines
+127
to
136
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When To improve the developer experience, I suggest adding a check that logs an error in development mode if fetchNextPage({ pageParam, ...options }: FetchNextPageOptions = {}): Promise<
InfiniteQueryObserverResult<TData, TError>
> {
if (
process.env.NODE_ENV !== 'production' &&
pageParam === undefined &&
!this.options.getNextPageParam
) {
console.error(
`'pageParam' is required for 'fetchNextPage' when 'getNextPageParam' is not defined.`,
)
}
return this.fetch({
...options,
meta: {
fetchMore: { direction: 'forward', pageParam },
},
})
} |
||
|
|
||
| fetchPreviousPage( | ||
| options?: FetchPreviousPageOptions, | ||
| ): Promise<InfiniteQueryObserverResult<TData, TError>> { | ||
| fetchPreviousPage({ | ||
| pageParam, | ||
| ...options | ||
| }: FetchPreviousPageOptions = {}): Promise< | ||
| InfiniteQueryObserverResult<TData, TError> | ||
| > { | ||
| return this.fetch({ | ||
| ...options, | ||
| meta: { | ||
| fetchMore: { direction: 'backward' }, | ||
| fetchMore: { direction: 'backward', pageParam }, | ||
| }, | ||
| }) | ||
| } | ||
|
Comment on lines
+138
to
150
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to I recommend adding a development-mode warning to alert the user when they call fetchPreviousPage({
pageParam,
...options
}: FetchPreviousPageOptions = {}): Promise<
InfiniteQueryObserverResult<TData, TError>
> {
if (
process.env.NODE_ENV !== 'production' &&
pageParam === undefined &&
!this.options.getPreviousPageParam
) {
console.error(
`'pageParam' is required for 'fetchPreviousPage' when 'getPreviousPageParam' is not defined.`,
)
}
return this.fetch({
...options,
meta: {
fetchMore: { direction: 'backward', pageParam },
},
})
} |
||
|
|
||
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.
The current logic for determining the
paramfor refetching has a potential bug. When!options.getNextPageParamis true, the expressionoldPageParams[currentPage] ?? options.initialPageParamis used for all pages. This is incorrect for pages other than the first one (currentPage > 0). IfoldPageParams[currentPage]isundefinedfor some reason, it will incorrectly fall back tooptions.initialPageParam, which could lead to fetching the wrong page data during a refetch.The fallback to
initialPageParamshould only apply to the very first page (currentPage === 0). For subsequent pages in manual mode, we should rely onoldPageParams[currentPage]exclusively. I've suggested a refactor of this logic to make it safer and more explicit.