feat: imperative infinite queries #6
Conversation
let's bring back imperative infinite queries, but only allow them in an all-or-nothing mode, dependent on if `getNextPageParam` has been passed
|
View your CI Pipeline Execution ↗ for commit 644677e.
☁️ Nx Cloud last updated this comment at |
|
Thanks for this! Can we get it merged? It's critical to our use case of using it with AgGrid's serverside row model. |
|
any update for this? |
WalkthroughThis update introduces enhancements to the infinite query logic and its associated type definitions and tests. The primary changes include allowing explicit Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant InfiniteQueryObserver
participant InfiniteQueryBehavior
participant Query
User->>InfiniteQueryObserver: fetchNextPage({ pageParam })
InfiniteQueryObserver->>Query: fetch({ meta: { fetchMore: { direction: 'forward', pageParam } } })
Query->>InfiniteQueryBehavior: handle fetchMore with direction and pageParam
InfiniteQueryBehavior->>Query: Return result with correct page data
Query->>InfiniteQueryObserver: Resolve fetch promise
InfiniteQueryObserver->>User: Return updated query result
Poem
✨ Finishing Touches
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. 🪧 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 full review |
✅ Actions performedFull review triggered. |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx(1 hunks)packages/query-core/src/__tests__/infiniteQueryObserver.test-d.tsx(1 hunks)packages/query-core/src/infiniteQueryBehavior.ts(4 hunks)packages/query-core/src/infiniteQueryObserver.ts(1 hunks)packages/query-core/src/query.ts(1 hunks)packages/query-core/src/types.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx (2)
packages/query-core/src/__tests__/utils.ts (1)
queryKey(24-27)packages/query-core/src/infiniteQueryObserver.ts (1)
InfiniteQueryObserver(27-198)
🪛 GitHub Actions: pr
packages/query-core/src/infiniteQueryObserver.ts
[error] 122-122: TypeScript error TS2339: Property 'pageParam' does not exist on type 'FetchNextPageOptions'.
🔇 Additional comments (10)
packages/query-core/src/query.ts (1)
92-92: Looks good! Adding support for explicit pageParam in fetchMore metadata.This addition to the
FetchMetainterface enables passing an explicitpageParamduring fetch operations, which is essential for implementing imperative infinite queries. This change works well with the corresponding updates in theinfiniteQueryObserverwhere thefetchNextPageandfetchPreviousPagemethods pass this parameter through.packages/query-core/src/types.ts (1)
274-274: Good change - making getNextPageParam optional aligns with the all-or-nothing imperative mode.Making
getNextPageParamoptional is a key change that enables imperative infinite queries. This aligns with the PR objective to support imperative infinite queries in an all-or-nothing mode, where ifgetNextPageParamis omitted, users must provide explicit page parameters. This also matches the existing optional nature ofgetPreviousPageParam.packages/query-core/src/__tests__/infiniteQueryObserver.test-d.tsx (2)
76-95: Great type test for the case when getNextPageParam is defined.This test correctly verifies that when
getNextPageParamis provided, thefetchNextPageandfetchPreviousPagemethods don't expect or allow apageParamargument, as it would be calculated by the function. This ensures type safety and prevents confusion in the API.
97-117: Excellent type test for the case when getNextPageParam is missing.This complementary test ensures that when
getNextPageParamis omitted, thefetchNextPageandfetchPreviousPagemethods require apageParamargument. This enforces the all-or-nothing behavior described in the PR objectives and ensures developers can't accidentally omit required parameters.packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx (1)
204-306: Well-designed test for imperative infinite queries functionality.This comprehensive test verifies that explicit
pageParamvalues are correctly applied when fetching pages. It covers the key scenarios:
- Initial fetch with the initialPageParam
- Fetching next page with explicit pageParam
- Fetching previous page with explicit pageParam
- Refetching - verifying that manually specified pageParams are preserved
The test provides good coverage of the imperative mode and ensures it works correctly with both new page fetches and refetches. The explicit assertions for each step make the test clear and maintainable.
packages/query-core/src/infiniteQueryBehavior.ts (5)
17-17: Simplified metadata accessGood change to directly access the
fetchMoreobject from the metadata, making the code cleaner and easier to understand.
84-86: Improved condition for fetching next/previous pagesThe condition now checks for both
fetchMoreexistence and the presence of old pages, which is more explicit and handles edge cases better. The direction determination is also cleaner, using thefetchMore.directionproperty directly.
91-95: Support for explicit page parametersThis is the core change that implements the imperative infinite query functionality. The code now checks if an explicit
pageParamwas provided infetchMore, and only falls back to using the page parameter functions if it wasn't. This aligns perfectly with the PR objective of supporting explicit page parameters when needed.
103-105: Handling optional getNextPageParam functionThis change adds support for cases where
getNextPageParamis not provided, falling back to using either the old page parameter or the initial page parameter. This is necessary to make the function optional and support the imperative infinite query mode.
142-148: Added optional chaining for getNextPageParamGood addition of optional chaining to handle cases where
getNextPageParamis undefined. This prevents runtime errors when operating in imperative mode without the parameter functions.
| fetchPreviousPage({ | ||
| pageParam, | ||
| ...options | ||
| }: FetchPreviousPageOptions = {}): Promise< | ||
| InfiniteQueryObserverResult<TData, TError> | ||
| > { | ||
| return this.fetch({ | ||
| ...options, | ||
| meta: { | ||
| fetchMore: { direction: 'backward' }, | ||
| fetchMore: { direction: 'backward', pageParam }, | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Support for explicit pageParam added to fetchPreviousPage
This change mirrors the update to fetchNextPage, adding support for explicit page parameters to the fetchPreviousPage method. This implementation maintains consistency between both pagination directions.
The same type issue applies here - ensure that FetchPreviousPageOptions type is updated to include the pageParam property.
| fetchNextPage({ pageParam, ...options }: FetchNextPageOptions = {}): Promise< | ||
| InfiniteQueryObserverResult<TData, TError> | ||
| > { | ||
| return this.fetch({ | ||
| ...options, | ||
| meta: { | ||
| fetchMore: { direction: 'forward' }, | ||
| fetchMore: { direction: 'forward', pageParam }, | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Good update to support imperative infinite queries
The fetchNextPage method now accepts an optional pageParam property which enables users to explicitly control the pagination, aligning with the PR's objective to reintroduce imperative infinite queries functionality.
A small issue to note: the pipeline shows a TypeScript error (TS2339) indicating that 'pageParam' doesn't exist on the FetchNextPageOptions type. The corresponding type definition should be updated.
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
|
/refacto-test |
1 similar comment
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
No review comments were generated during this review iteration of the PR. |
Security Review: Infinite Queries👍 Well Done
📝 Additional Comments
|
| const param = | ||
| fetchMore.pageParam === undefined | ||
| ? pageParamFn(options, oldData) | ||
| : fetchMore.pageParam |
There was a problem hiding this comment.
Missing PageParam Validation
User-provided pageParam is used without validation. Attackers could inject malicious values causing data exposure or application crashes. Especially risky when pageParam flows to database queries.
| const param = | |
| fetchMore.pageParam === undefined | |
| ? pageParamFn(options, oldData) | |
| : fetchMore.pageParam | |
| const param = | |
| fetchMore.pageParam === undefined | |
| ? pageParamFn(options, oldData) | |
| : validatePageParam(fetchMore.pageParam, options) | |
| // Add this function to validate incoming pageParam values | |
| function validatePageParam(pageParam: unknown, options: InfiniteQueryPageParamsOptions<TData>): unknown { | |
| // Implement validation based on expected pageParam type | |
| // Example: type checking, range validation, sanitization | |
| return pageParam; // Return validated/sanitized value | |
| } |
Standards
- CWE-20
- OWASP-A03
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: InfiniteQuery Enhancement👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| const param = | ||
| fetchMore.pageParam === undefined | ||
| ? pageParamFn(options, oldData) | ||
| : fetchMore.pageParam |
There was a problem hiding this comment.
Missing Null Check
No null check for pageParamFn before invocation. If getNextPageParam/getPreviousPageParam is undefined, this will cause runtime errors when fetchMore.pageParam is undefined.
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| currentPage === 0 || !options.getNextPageParam | ||
| ? (oldPageParams[currentPage] ?? options.initialPageParam) |
There was a problem hiding this comment.
Missing getNextPageParam Check
Logic incorrectly treats missing getNextPageParam as a condition to use oldPageParams instead of calculating next page. This breaks expected behavior for subsequent pages when getNextPageParam is undefined.
Standards
- Algorithm-Correctness-Conditional
- Logic-Verification-Flow
- Business-Rule-Pagination
| ? options.getNextPageParam?.( | ||
| pages[lastIndex], |
There was a problem hiding this comment.
Incorrect Optional Chaining
Optional chaining added to getNextPageParam but the function is already checked for existence on line 104. This creates inconsistent logic flow where the function is treated as both optional and required.
Standards
- Logic-Verification-Consistency
- Algorithm-Correctness-Nullability
- Business-Rule-Validation
| fetchNextPage({ pageParam, ...options }: FetchNextPageOptions = {}): Promise< | ||
| InfiniteQueryObserverResult<TData, TError> | ||
| > { | ||
| return this.fetch({ | ||
| ...options, | ||
| meta: { | ||
| fetchMore: { direction: 'forward' }, | ||
| fetchMore: { direction: 'forward', pageParam }, | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Optional Parameter Type
The pageParam parameter is destructured but its type isn't explicitly defined in the function signature. This could lead to maintainability issues when other developers need to understand the expected type.
Standards
- Clean-Code-Type-Safety
- Clean-Code-Function-Signatures
| * The result will also be used to determine the value of `hasNextPage`. | ||
| */ | ||
| getNextPageParam: GetNextPageParamFunction<TPageParam, TQueryFnData> | ||
| getNextPageParam?: GetNextPageParamFunction<TPageParam, TQueryFnData> |
There was a problem hiding this comment.
Type Definition Update
Making getNextPageParam optional changes the contract significantly. This requires comprehensive documentation updates and might affect consumers who expect this function to always exist.
Standards
- SOLID-LSP
- Clean-Code-Interface-Design
| currentPage === 0 || !options.getNextPageParam | ||
| ? (oldPageParams[currentPage] ?? options.initialPageParam) |
There was a problem hiding this comment.
Optional GetNextPageParam Check
The check for !options.getNextPageParam is new but will always be false when refetching since getNextPageParam is used later. This creates an unnecessary condition check on every page fetch. Consider simplifying this logic or adding a comment explaining the purpose of this check for better code clarity.
Standards
- ISO-IEC-25010-Performance-Time-Behaviour
- Algorithm-Opt-Conditional-Evaluation
| currentPage === 0 || !options.getNextPageParam | ||
| ? (oldPageParams[currentPage] ?? options.initialPageParam) |
There was a problem hiding this comment.
Inconsistent PageParam Logic
Logic change creates inconsistency in pageParam selection. For currentPage=0, oldPageParams[0] was replaced with oldPageParams[currentPage], but for currentPage>0 with getNextPageParam defined, the logic still uses getNextPageParam without considering oldPageParams[currentPage].
Standards
- Algorithm-Correctness-Conditional-Logic
- Logic-Verification-Consistency
- Business-Rule-State-Management
| fetchPreviousPage({ | ||
| pageParam, | ||
| ...options | ||
| }: FetchPreviousPageOptions = {}): Promise< | ||
| InfiniteQueryObserverResult<TData, TError> | ||
| > { |
There was a problem hiding this comment.
Missing PageParam Validation
fetchPreviousPage accepts pageParam without validation. Invalid pageParam values could cause unexpected behavior or failures during pagination operations.
Standards
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- ISO-IEC-25010-Reliability-Fault-Tolerance
| const param = | ||
| fetchMore.pageParam === undefined | ||
| ? pageParamFn(options, oldData) | ||
| : fetchMore.pageParam |
There was a problem hiding this comment.
Optimize Refetch Logic
The current implementation checks fetchMore.pageParam === undefined to determine whether to use the pageParamFn or the explicit pageParam. This creates unnecessary function calls when pageParam is provided. Consider short-circuit evaluation to avoid calling pageParamFn entirely when pageParam is provided, reducing computational overhead during pagination operations.
Standards
- ISO-IEC-25010-Performance-Time-Behaviour
- Algorithm-Opt-Short-Circuit
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Infinite Query Manual PageParam Support👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| * The result will also be used to determine the value of `hasNextPage`. | ||
| */ | ||
| getNextPageParam: GetNextPageParamFunction<TPageParam, TQueryFnData> | ||
| getNextPageParam?: GetNextPageParamFunction<TPageParam, TQueryFnData> |
There was a problem hiding this comment.
Optional getNextPageParam Without Code Updates
Making getNextPageParam optional without updating all dependent code could cause runtime failures. Some code paths may assume this function exists without checking, leading to potential undefined behavior.
| getNextPageParam?: GetNextPageParamFunction<TPageParam, TQueryFnData> | |
| getNextPageParam: GetNextPageParamFunction<TPageParam, TQueryFnData> |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| fetchNextPage({ pageParam, ...options }: FetchNextPageOptions = {}): Promise< | ||
| InfiniteQueryObserverResult<TData, TError> | ||
| > { | ||
| return this.fetch({ | ||
| ...options, | ||
| meta: { | ||
| fetchMore: { direction: 'forward' }, | ||
| fetchMore: { direction: 'forward', pageParam }, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| fetchPreviousPage( | ||
| options?: FetchPreviousPageOptions, | ||
| ): Promise<InfiniteQueryObserverResult<TData, TError>> { | ||
| fetchPreviousPage({ | ||
| pageParam, | ||
| ...options | ||
| }: FetchPreviousPageOptions = {}): Promise< | ||
| InfiniteQueryObserverResult<TData, TError> | ||
| > { |
There was a problem hiding this comment.
Missing pageParam Validation
No validation ensures pageParam is provided when getNextPageParam is missing. Without validation, fetchNextPage/fetchPreviousPage could silently pass undefined pageParam when required, causing runtime errors.
| fetchNextPage({ pageParam, ...options }: FetchNextPageOptions = {}): Promise< | |
| InfiniteQueryObserverResult<TData, TError> | |
| > { | |
| return this.fetch({ | |
| ...options, | |
| meta: { | |
| fetchMore: { direction: 'forward' }, | |
| fetchMore: { direction: 'forward', pageParam }, | |
| }, | |
| }) | |
| } | |
| fetchPreviousPage( | |
| options?: FetchPreviousPageOptions, | |
| ): Promise<InfiniteQueryObserverResult<TData, TError>> { | |
| fetchPreviousPage({ | |
| pageParam, | |
| ...options | |
| }: FetchPreviousPageOptions = {}): Promise< | |
| InfiniteQueryObserverResult<TData, TError> | |
| > { | |
| fetchNextPage({ pageParam, ...options }: FetchNextPageOptions = {}): Promise< | |
| InfiniteQueryObserverResult<TData, TError> | |
| > { | |
| if (!this.options.getNextPageParam && pageParam === undefined) { | |
| throw new Error('pageParam is required when getNextPageParam is not defined') | |
| } | |
| return this.fetch({ | |
| ...options, | |
| meta: { | |
| fetchMore: { direction: 'forward', pageParam }, | |
| }, | |
| }) | |
| } |
Standards
- Business-Rule-Validation
- Logic-Verification-Parameter-Validation
- Algorithm-Correctness-Input-Validation
| const param = | ||
| fetchMore.pageParam === undefined | ||
| ? pageParamFn(options, oldData) | ||
| : fetchMore.pageParam |
There was a problem hiding this comment.
Input Validation Missing
Manual pageParam is used without validation. Attackers could inject malicious values causing unexpected behavior or data exposure. Add validation before using external pageParam values.
Standards
- CWE-20
- OWASP-A03
| ? options.getNextPageParam?.( | ||
| pages[lastIndex], | ||
| pages, | ||
| pageParams[lastIndex], |
There was a problem hiding this comment.
Missing Null Check
Optional chaining on getNextPageParam but no fallback if undefined. Could return undefined when called, potentially causing runtime errors in consumers expecting a value.
| ? options.getNextPageParam?.( | |
| pages[lastIndex], | |
| pages, | |
| pageParams[lastIndex], | |
| ? (options.getNextPageParam?.( | |
| pages[lastIndex], | |
| pages, | |
| pageParams[lastIndex], | |
| ) ?? null) |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| const param = | ||
| fetchMore.pageParam === undefined | ||
| ? pageParamFn(options, oldData) | ||
| : fetchMore.pageParam |
There was a problem hiding this comment.
Optional Parameter Type
The pageParam is conditionally used but lacks explicit typing. Adding proper type annotations would improve code clarity and maintainability, especially for future developers working with this API.
Standards
- Clean-Code-Type-Safety
- Clean-Code-Explicit-Intent
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Infinite Queries PageParam Enhancement👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| ? options.getNextPageParam?.( | ||
| pages[lastIndex], | ||
| pages, | ||
| pageParams[lastIndex], |
There was a problem hiding this comment.
Optional getNextPageParam
Making getNextPageParam optional could cause runtime issues if code expects it to be defined. The optional chaining operator allows it to be undefined, but code might still assume a valid function exists.
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| currentPage === 0 || !options.getNextPageParam | ||
| ? (oldPageParams[currentPage] ?? options.initialPageParam) |
There was a problem hiding this comment.
Optional getNextPageParam
The condition checks for missing getNextPageParam but uses oldPageParams[currentPage] which may be undefined. This creates inconsistent parameter handling that could lead to unexpected behavior when refetching pages.
Standards
- Algorithm-Correctness-Null-Safety
- Logic-Verification-Control-Flow
- Business-Rule-State-Consistency
| fetchNextPage({ pageParam, ...options }: FetchNextPageOptions = {}): Promise< | ||
| InfiniteQueryObserverResult<TData, TError> | ||
| > { | ||
| return this.fetch({ | ||
| ...options, | ||
| meta: { | ||
| fetchMore: { direction: 'forward' }, | ||
| fetchMore: { direction: 'forward', pageParam }, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Missing Type Validation
The pageParam is passed directly to fetchMore without type validation. Since this is a new parameter that could be undefined, null, or any other value, it should be validated before use to prevent potential type-related issues in the pagination logic.
Standards
- Business-Rule-Input-Validation
- Logic-Verification-Data-Flow
- Algorithm-Correctness-Type-Safety
| const param = | ||
| fetchMore.pageParam === undefined | ||
| ? pageParamFn(options, oldData) | ||
| : fetchMore.pageParam |
There was a problem hiding this comment.
Potential Parameter Override
The implementation allows overriding pageParam with user-provided values without validation. This could potentially lead to injection attacks if pageParam is used in sensitive operations like database queries or file operations without proper validation.
Standards
- CWE-20
- OWASP-A03
| * The result will also be used to determine the value of `hasNextPage`. | ||
| */ | ||
| getNextPageParam: GetNextPageParamFunction<TPageParam, TQueryFnData> | ||
| getNextPageParam?: GetNextPageParamFunction<TPageParam, TQueryFnData> |
There was a problem hiding this comment.
Optional getNextPageParam
Making getNextPageParam optional changes the core API contract. This requires careful consideration of backward compatibility and may lead to inconsistent behavior in existing code that expects getNextPageParam to always be defined.
Standards
- Maintainability-Quality-API-Stability
- Clean-Code-Interface-Design
| const param = | ||
| fetchMore.pageParam === undefined | ||
| ? pageParamFn(options, oldData) | ||
| : fetchMore.pageParam |
There was a problem hiding this comment.
Conditional Logic Complexity
The nested ternary operator increases cognitive complexity. This pattern could be simplified with a more direct approach like destructuring with defaults or early returns, making the code more readable and maintainable.
Standards
- Clean-Code-Simplicity
- Maintainability-Quality-Complexity
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Infinite Query Enhancement👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| currentPage === 0 || !options.getNextPageParam | ||
| ? (oldPageParams[currentPage] ?? options.initialPageParam) |
There was a problem hiding this comment.
Incorrect PageParam Selection
The condition incorrectly uses currentPage === 0 OR !options.getNextPageParam, which means even if getNextPageParam is defined, it will still use oldPageParams for the first page. This breaks the expected behavior where getNextPageParam should be used for all pages except the first one.
currentPage === 0
? (oldPageParams[0] ?? options.initialPageParam)
Commitable Suggestion
| currentPage === 0 || !options.getNextPageParam | |
| ? (oldPageParams[currentPage] ?? options.initialPageParam) | |
| currentPage === 0 | |
| ? (oldPageParams[0] ?? options.initialPageParam) |
Standards
- Algorithm-Correctness-Conditional-Logic
- Business-Rule-State-Consistency
- Logic-Verification-Data-Flow
| ? options.getNextPageParam?.( | ||
| pages[lastIndex], | ||
| pages, | ||
| pageParams[lastIndex], |
There was a problem hiding this comment.
Optional getNextPageParam
The getNextPageParam function is now optional but used without null check at line 105. This could cause runtime errors if getNextPageParam is undefined but the code at line 105 attempts to call it.
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| if (fetchMore && oldPages.length) { | ||
| const previous = fetchMore.direction === 'backward' |
There was a problem hiding this comment.
Inconsistent Direction Handling
The code now uses fetchMore object but inconsistently handles the direction property. In some places it checks for direction existence, in others it assumes direction is defined. This inconsistency creates maintenance challenges and potential bugs when modifying direction-related logic.
if (fetchMore?.direction && oldPages.length) {
const previous = fetchMore.direction === 'backward'
Commitable Suggestion
| if (fetchMore && oldPages.length) { | |
| const previous = fetchMore.direction === 'backward' | |
| if (fetchMore?.direction && oldPages.length) { | |
| const previous = fetchMore.direction === 'backward' |
Standards
- Clean-Code-Consistency
- SOLID-SRP
- Maintainability-Quality-Predictability
| currentPage === 0 || !options.getNextPageParam | ||
| ? (oldPageParams[currentPage] ?? options.initialPageParam) | ||
| : getNextPageParam(options, result) | ||
| if (currentPage > 0 && param == null) { |
There was a problem hiding this comment.
Missing Null Check
The code attempts to call getNextPageParam() at line 104 even when options.getNextPageParam is undefined (checked at line 103). This inconsistency could lead to runtime errors when getNextPageParam is undefined.
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| fetchNextPage({ pageParam, ...options }: FetchNextPageOptions = {}): Promise< | ||
| InfiniteQueryObserverResult<TData, TError> | ||
| > { | ||
| return this.fetch({ | ||
| ...options, | ||
| meta: { | ||
| fetchMore: { direction: 'forward' }, | ||
| fetchMore: { direction: 'forward', pageParam }, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| fetchPreviousPage( | ||
| options?: FetchPreviousPageOptions, | ||
| ): Promise<InfiniteQueryObserverResult<TData, TError>> { | ||
| fetchPreviousPage({ | ||
| pageParam, | ||
| ...options | ||
| }: FetchPreviousPageOptions = {}): Promise< | ||
| InfiniteQueryObserverResult<TData, TError> | ||
| > { |
There was a problem hiding this comment.
Optional Parameter Handling
Inconsistent parameter destructuring patterns between fetchNextPage and fetchPreviousPage methods. The first uses inline destructuring while the second uses multi-line format. This inconsistency makes the code harder to maintain as developers need to mentally parse different patterns.
Standards
- Clean-Code-Consistency
- Maintainability-Quality-Readability
4 participants
let's bring back imperative infinite queries, but only allow them in an all-or-nothing mode, dependent on if
getNextPageParamhas been passedSummary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor
Documentation