-
Notifications
You must be signed in to change notification settings - Fork 0
fix(react): allow using enabled when using useQuery().promise
#7
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
base: main
Are you sure you want to change the base?
Changes from all commits
3b569b4
7bf0f38
e01497a
1455669
2ad266e
b7cf7fc
b222b14
a7994c2
f1daa83
a269d27
3445626
7669e03
46b7e06
2f69183
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 | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -70,9 +70,7 @@ export function useBaseQuery< | |||||||||||||||||||
| useClearResetErrorBoundary(errorResetBoundary) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // this needs to be invoked before creating the Observer because that can create a cache entry | ||||||||||||||||||||
| const isNewCacheEntry = !client | ||||||||||||||||||||
| .getQueryCache() | ||||||||||||||||||||
| .get(defaultedOptions.queryHash) | ||||||||||||||||||||
| const cacheEntry = client.getQueryCache().get(defaultedOptions.queryHash) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const [observer] = React.useState( | ||||||||||||||||||||
| () => | ||||||||||||||||||||
|
|
@@ -143,7 +141,16 @@ export function useBaseQuery< | |||||||||||||||||||
| !isServer && | ||||||||||||||||||||
| willFetch(result, isRestoring) | ||||||||||||||||||||
| ) { | ||||||||||||||||||||
| const promise = isNewCacheEntry | ||||||||||||||||||||
| // This fetching in the render should likely be done as part of the getOptimisticResult() considering https://github.com/TanStack/query/issues/8507 | ||||||||||||||||||||
|
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. Logic Issue: The condition for 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. Inline Comment PlacementThe comment is placed directly before the implementation logic it describes, breaking the flow of code. Moving this comment above the entire block (before line 143) would improve readability while maintaining the context. Standards
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. Inline Comment ClarityThe comment indicates a potential architectural improvement but doesn't explain why the current implementation is problematic. Adding context about the architectural concern would improve future maintainability when revisiting this code. Standards
|
||||||||||||||||||||
| const state = cacheEntry?.state | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const shouldFetch = | ||||||||||||||||||||
|
Comment on lines
+144
to
+147
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. Race Condition RiskInsufficient state validation creates race condition when enabled changes. Concurrent renders could trigger multiple fetches with inconsistent state. Potential memory leaks and unpredictable component behavior.
Suggested change
Standards
|
||||||||||||||||||||
| !state || | ||||||||||||||||||||
| (state.data === undefined && | ||||||||||||||||||||
| state.status === 'pending' && | ||||||||||||||||||||
| state.fetchStatus === 'idle') | ||||||||||||||||||||
|
Comment on lines
+147
to
+151
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. Incorrect Fetch ConditionThe fetch condition logic has been changed from a simple cache entry check to a more complex state-based condition. However, the new condition will trigger a fetch when state.data is undefined AND status is pending AND fetchStatus is idle, which is too restrictive and may prevent necessary fetches in other valid scenarios. Commitable Suggestion
Suggested change
Standards
Comment on lines
+147
to
+151
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. Inconsistent Fetch ConditionThe shouldFetch condition doesn't account for enabled state changes. When enabled changes from false to true, the condition may not trigger a fetch if state exists with pending status, causing stale or missing data. This creates inconsistent behavior when toggling enabled state. Standards
Comment on lines
+147
to
+151
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. Redundant State CheckThe condition checks for !state first, then accesses state properties without null check in the second condition. If state is null/undefined, the second condition would cause runtime errors. This creates potential for uncaught exceptions during state transitions. Standards
Comment on lines
+147
to
+151
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. Redundant State CheckThe shouldFetch condition performs redundant checks that could be simplified. When state is undefined (!state), the subsequent condition is unnecessary. This creates extra boolean operations on every render affecting component performance under high frequency renders. Standards
Comment on lines
+147
to
+151
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. Unnecessary Fetch ConditionThe shouldFetch condition checks for pending status but idle fetchStatus, which is contradictory. This could cause unnecessary fetches or missed fetches in edge cases, affecting query reliability and causing inconsistent behavior. Standards
Comment on lines
+144
to
+151
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. Incorrect Fetch ConditionThe original condition used isNewCacheEntry to determine fetching, but the new logic checks state properties. This creates a logical inconsistency where a query with pending status but non-idle fetchStatus would be incorrectly skipped, potentially causing stale data to be used. Standards
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| const promise = shouldFetch | ||||||||||||||||||||
|
Comment on lines
+144
to
+153
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. Inconsistent Fetch ConditionThe condition for fetching changed from 'isNewCacheEntry' to 'shouldFetch', but the variable 'isNewCacheEntry' is no longer used. This creates inconsistency between variable declaration and usage, potentially causing unexpected behavior when the enabled state changes. Standards
|
||||||||||||||||||||
| ? // Fetch immediately on render in order to ensure `.promise` is resolved even if the component is unmounted | ||||||||||||||||||||
| fetchOptimistic(defaultedOptions, observer, errorResetBoundary) | ||||||||||||||||||||
| : // subscribe to the "cache promise" so that we can finalize the currentThenable once data comes in | ||||||||||||||||||||
|
|
||||||||||||||||||||
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.
Incomplete Test Coverage
The test verifies queryFn is called exactly once, but doesn't test the case where enabled changes from true to false and back to true. This leaves a logical gap in test coverage since the promise reuse behavior isn't fully verified across multiple enabled state transitions.
Standards