[WIP] New useQueryState Hook #5
Conversation
the value of the ref can get out of sync if we only write the ref in the subscribe function
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 05e18d3. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 05e18d3:
|
|
There appear to be two failing tests on this PR and I can't seem to figure out why they're failing. Namely, they are the While debugging, I've noticed the following:
I can't seem to figure out why this is the case or why it works in the other tests, but not these. It also appears that there's some timing issues with |
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #6758 +/- ##
===========================================
+ Coverage 41.78% 85.09% +43.30%
===========================================
Files 178 24 -154
Lines 7017 322 -6695
Branches 1421 83 -1338
===========================================
- Hits 2932 274 -2658
+ Misses 3722 39 -3683
+ Partials 363 9 -354 ☔ View full report in Codecov by Sentry. |
WalkthroughThe changes introduce a new React hook, Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant useQueryState
participant QueryClient
participant QueryCache
Component->>useQueryState: Call with options, queryClient?
useQueryState->>QueryClient: Get QueryCache
useQueryState->>QueryCache: Subscribe to updates
QueryCache-->>useQueryState: Notify on change
useQueryState->>Component: Return filtered query states (reactively)
sequenceDiagram
participant Component
participant useIsFetching
participant useQueryState
Component->>useIsFetching: Call with filters, queryClient?
useIsFetching->>useQueryState: Call with filters + fetchStatus: 'fetching'
useQueryState-->>useIsFetching: Return array of fetching queries
useIsFetching->>Component: Return length of array (number of fetching)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🧰 Additional context used🧬 Code Graph Analysis (1)packages/react-query/src/__tests__/useMutationState.test.tsx (2)
🪛 GitHub Actions: prpackages/react-query/src/useQueryState.ts[error] 39-39: TypeScript error TS2554: Expected 1 arguments, but got 0. 🔇 Additional comments (9)
✨ 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: 3
🧹 Nitpick comments (2)
packages/react-query/src/useQueryState.ts (2)
48-66: Ensure consistent result reference even with edge casesThe implementation of
useSyncExternalStorewith refs is good, but there's a potential issue with the server-side rendering getter function.The server-side rendering getter (third parameter) is using
result.currentdirectly, which should be fine in most cases. However, if there are differences between server and client environments, you might want to ensure that the same computation logic is used for both:return React.useSyncExternalStore( React.useCallback( (onStoreChange) => queryCache.subscribe(notifyManager.batchCalls(onStoreChange)), [queryCache], ), () => { const nextResult = replaceEqualDeep( result.current, getResult(queryCache, optionsRef.current), ) if (result.current !== nextResult) { result.current = nextResult } return result.current }, - () => result.current, + () => { + // Use the same computation logic for server rendering + if (!result.current) { + result.current = getResult(queryCache, optionsRef.current) + } + return result.current + }, )!
16-19: Consider documenting the generic type parameterThe
QueryStateOptionstype uses a generic parameterTResultwhich defaults toQueryState, but there's no JSDoc to explain what this represents.Consider adding documentation:
+/** + * Options for useQueryState hook. + * @template TResult The return type of the selection function, defaults to QueryState. + */ type QueryStateOptions<TResult = QueryState> = { filters?: QueryFilters select?: (query: Query<unknown, DefaultError, unknown, QueryKey>) => TResult }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/react-query/src/__tests__/useIsFetching.test.tsx(1 hunks)packages/react-query/src/__tests__/useMutationState.test.tsx(3 hunks)packages/react-query/src/index.ts(1 hunks)packages/react-query/src/useIsFetching.ts(1 hunks)packages/react-query/src/useMutationState.ts(1 hunks)packages/react-query/src/useQueryState.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/react-query/src/useIsFetching.ts (2)
packages/react-query/src/index.ts (2)
useIsFetching(41-41)useQueryState(43-43)packages/react-query/src/useQueryState.ts (1)
useQueryState(33-67)
🪛 GitHub Actions: pr
packages/react-query/src/useQueryState.ts
[error] 39-39: TypeScript error TS2554: Expected 1 arguments, but got 0.
🔇 Additional comments (8)
packages/react-query/src/index.ts (1)
43-43: LGTM! Correct export of the new hook.The new hook is properly exported, making it available in the public API.
packages/react-query/src/__tests__/useIsFetching.test.tsx (1)
207-209: Hook call order changed - intentional refactoring?The
useIsFetchinghook call has been moved before theuseQuerycall. This change is related to the internal refactoring whereuseIsFetchingnow uses the newuseQueryStatehook.Please verify this change is intentional. If the order of hook calls matters in the new implementation, it would be helpful to add a comment explaining why:
function Page() { + // useIsFetching must be called before useQuery due to internal implementation details const isFetching = useIsFetching({}, queryClient) useQuery(packages/react-query/src/__tests__/useMutationState.test.tsx (2)
69-77: Good improvement using React.memo to isolate hook behaviorUsing
React.memoto prevent re-renders from other hooks is a good practice that helps isolate the behavior of the hook being tested. This makes the test more reliable and focused.
110-120: Consistent pattern applied to the second test caseThe same memoization pattern is correctly applied to this test case as well, ensuring consistent behavior across tests.
packages/react-query/src/useIsFetching.ts (2)
2-2: Good import of the new useQueryState hook.The integration of the new
useQueryStatehook aligns with the PR objective of refactoring existing hooks to use the new centralized query state management.
9-12:❓ Verification inconclusive
Clean implementation using the new hook.
The implementation is concise and follows good composition principles by delegating the subscription logic to
useQueryState. The filters are properly extended withfetchStatus: 'fetching'to maintain the original behavior.Since there are reported test failures in the PR comments specifically related to
useIsFetching, verify that the timing of updates is consistent with the previous implementation:
🏁 Script executed:
#!/bin/bash # Find the failing tests related to useIsFetching rg -A 5 "useIsFetching.*test" packages/Length of output: 41
Please verify update timing for useIsFetching
I couldn’t locate any existing tests that cover
useIsFetching, so it’s unclear whether the new implementation still triggers updates at the same moment as before. Please manually confirm:
- The
.lengthreturned byuseQueryState({ filters: { …filters, fetchStatus: 'fetching' } }, queryClient)updates synchronously with changes infetchStatus.- Consumer components see the same re‑render timing as the previous
useIsFetchinghook.packages/react-query/src/useMutationState.ts (2)
67-67: Simplified subscription callback.Good refactoring to simplify the subscription logic by focusing solely on batching notification calls without additional state management logic.
70-80: Improved state update logic in snapshot getter.Well-structured refactoring that moves the result computation and comparison logic from the subscription callback to the snapshot getter function. This follows React's recommended patterns for using
useSyncExternalStoreand creates a more maintainable separation of concerns.The implementation correctly:
- Computes the next result using
getResult- Performs deep equality comparison with
replaceEqualDeep- Updates the cached result only when necessary
- Returns the current result
This approach should improve consistency across query-related hooks in the library.
| ): Array<TResult> { | ||
| const queryCache = useQueryClient(queryClient).getQueryCache() | ||
| const optionsRef = React.useRef(options) | ||
| const result = React.useRef<Array<TResult>>() |
There was a problem hiding this comment.
Fix TypeScript error: Initial value missing for useRef
The TypeScript error indicates that React.useRef expects an initial value, but none is provided here. This is causing the build failure.
Apply this fix:
- const result = React.useRef<Array<TResult>>()
+ const result = React.useRef<Array<TResult>>([])📝 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 result = React.useRef<Array<TResult>>() | |
| const result = React.useRef<Array<TResult>>([]) |
🧰 Tools
🪛 GitHub Actions: pr
[error] 39-39: TypeScript error TS2554: Expected 1 arguments, but got 0.
| export function useQueryState<TResult = QueryState>( | ||
| options: QueryStateOptions<TResult> = {}, | ||
| queryClient?: QueryClient, | ||
| ): Array<TResult> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add JSDoc documentation for useQueryState
This is a new public API, and it should have descriptive documentation to help users understand its purpose and usage.
Add JSDoc documentation:
+/**
+ * Hook that provides access to the state of queries in the cache.
+ *
+ * @template TResult The return type of the selection function, defaults to QueryState.
+ * @param options Configuration object to filter and transform query results
+ * @param queryClient Optional QueryClient instance, will use the default from context if not provided
+ * @returns An array of query states or selected results
+ *
+ * @example
+ * // Get all fetching queries
+ * const fetchingQueries = useQueryState({
+ * filters: { fetchStatus: 'fetching' }
+ * })
+ *
+ * @example
+ * // Get only the data from specific queries
+ * const queryData = useQueryState({
+ * filters: { queryKey: ['todos'] },
+ * select: (query) => query.state.data
+ * })
+ */
export function useQueryState<TResult = QueryState>(
options: QueryStateOptions<TResult> = {},
queryClient?: QueryClient,
): Array<TResult> {📝 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.
| export function useQueryState<TResult = QueryState>( | |
| options: QueryStateOptions<TResult> = {}, | |
| queryClient?: QueryClient, | |
| ): Array<TResult> { | |
| /** | |
| * Hook that provides access to the state of queries in the cache. | |
| * | |
| * @template TResult The return type of the selection function, defaults to QueryState. | |
| * @param options Configuration object to filter and transform query results | |
| * @param queryClient Optional QueryClient instance, will use the default from context if not provided | |
| * @returns An array of query states or selected results | |
| * | |
| * @example | |
| * // Get all fetching queries | |
| * const fetchingQueries = useQueryState({ | |
| * filters: { fetchStatus: 'fetching' } | |
| * }) | |
| * | |
| * @example | |
| * // Get only the data from specific queries | |
| * const queryData = useQueryState({ | |
| * filters: { queryKey: ['todos'] }, | |
| * select: (query) => query.state.data | |
| * }) | |
| */ | |
| export function useQueryState<TResult = QueryState>( | |
| options: QueryStateOptions<TResult> = {}, | |
| queryClient?: QueryClient, | |
| ): Array<TResult> { |
| if (!result.current) { | ||
| result.current = getResult(queryCache, options) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Revisit initialization logic
Since you're initializing the result.current conditionally, this creates a potential race condition where the initial state might be undefined if accessed during the first render cycle before this check runs.
Consider either:
- Setting the initial value directly in the
useRefcall (like the fix above), or - Add a null check when returning the result to handle potential undefined value
- if (!result.current) {
- result.current = getResult(queryCache, options)
- }
+ // Initialize with current results when ref is empty
+ result.current = result.current || getResult(queryCache, options)📝 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.
| if (!result.current) { | |
| result.current = getResult(queryCache, options) | |
| } | |
| // Initialize with current results when ref is empty | |
| result.current = result.current || getResult(queryCache, options) |
Closes TanStack#6756
TODO:
useQueryStatelogic touseMutationStateuseQueryStateuseIsFetchingas deprecateduseIsMutatingas deprecatedSummary by CodeRabbit
New Features
Refactor
Tests