Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 7462554 The changes in this PR will be included in the next version bump. This PR includes changesets to release 24 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 27.27% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 7 files. (1 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the primary change: creating a new observer when the queryClient changes. Linked Issues check Issue #5458is closed. It provides historical context only. No active directly linked issue supplies coding requirements for this pull request.Out of Scope Changes check The changes stay within the stated React adapter scope. The new useClientObserver implementation, useQuery and useQueries integration, related tests, hydration and persistence test updates, and the ch… Description check The pull request description follows the required template. It explains the changes and motivation, completes the checklist, documents testing and its limitation, and confirms that a changeset was gen… Full details: Out of Scope Changes check
-
Explanation
The changes stay within the stated React adapter scope. The new useClientObserver implementation, useQuery and useQueries integration, related tests, hydration and persistence test updates, and the changeset support the client-switch behavior. The pull request does not modify query-core or include the identified preact follow-up.
Full details: Description check
-
Explanation
The pull request description follows the required template. It explains the changes and motivation, completes the checklist, documents testing and its limitation, and confirms that a changeset was generated for published code.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 27.27% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 7 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Comment @coderabbitai help to get the list of available commands.
| () => | ||
| const observer = useClientObserver( | ||
| client, | ||
| (observerClient) => |
There was a problem hiding this comment.
this argument isn't required (it literally is the client variable above, on every render), but I personally like it because it makes lack of a dependency array (which we don't need, we don't use any hooks that need one) obviously unnecessary (otherwise you get suspicious of the hook internals)
| ? observerState | ||
| : { client, observer: createObserver(client) } | ||
|
|
||
| if (nextObserverState !== observerState) { |
There was a problem hiding this comment.
Called out in top-level description as well, but if somebody is mistakenly sending in a new client on every render, this would just keep rewriting state
That is pretty explicitly incorrect usage, but this might be the first place we would trigger a "too many renders" error?
IMHO acceptable (and this would just be a patch still, not something like a major)
🎯 Changes
Fixes #5458
useQueryanduseQueriescreate their observer once, with the firstqueryClientthey render with. If the client changes later (a differentQueryClientProviderclient, or a differentqueryClientargument), the hook takes options and cache lookups from the new client, but the result, the subscription and fetches still go through the old one.In #5458 (comment) the suggestion was a
setClienton the observer. I tried that first, but calling it during render changes the committed observer. If that render never commits (an interrupted transition, or a sibling that suspends or throws), the mounted observer is left on the new client, possibly subscribed to its query or fetching it. Calling it in an effect instead means the first render after the switch still shows the old client's data.So this does what the Solid adapter does (#11920): when the client changes, the hook creates a new observer for it.
useBaseQueryanduseQueriesget their observer from a new internaluseClientObserver(client, createObserver)hook instead ofuseState. It keeps{ client, observer }in state. When the render's client differs fromstate.client, it creates a new observer for that client, uses it for that render, and stores it with React's "adjust state while rendering" pattern. React drops that update if the render is thrown away, so a render that doesn't commit never touches the committed observer.useSyncExternalStore's subscribe callback and thesetOptionseffect already depend onobserver, so after the commit React unsubscribes the old observer and subscribes the new one.Tests:
useQueryanduseQueries: render with client A, then client B (through the provider and through thequeryClientargument), with different data for the same key. The hook shows B's data on the first render with B, followsB.setQueryData, and ignoresA.setQueryData. There's also auseQuerytest where B has no data, so it fetches into B.useQuerytest where a transition switches to client B and a sibling suspends, so the transition never commits. The mounted hook still reads A and still updates from A, B's query has no observer, and nothing is fetched. This test fails if the observer is switched during render.HydrationBoundary"should hydrate queries to new cache if cache changes": after the switch it now shows the data hydrated into the new cache (stringCached), then the refetch result. The timer goes from 20ms to 21ms to let the 20msqueryFnfinish.PersistQueryClientProvider"should be able to persist into multiple clients": the provider only restores into the first client, and the test swaps the client right away. The hook used to show the restored data from the first client's cache, but fetched with the second client'squeryFn. It now follows the second client, which was never restored, so it goes from pending straight toqueryFn2. Restoring into a replaced client would be a separate change to the provider. The recorded states also have one more entry, because the render that switches observers runs twice.Tradeoff: the new observer starts fresh, so previous data,
placeholderData'spreviousData, and tracked props don't carry over from the old client.This also changes what happens with an unstable client, such as
new QueryClient()created during render. Previously the hook ignored every client after the first, so that mistake went unnoticed. Now:useQuery(options, new QueryClient())), each re-render that the hook triggers to store the new observer creates another client, so React throws "Too many re-renders" on the first render.<QueryClientProvider client={new QueryClient()}>), the hook settles, but each parent render gives it a new observer on an empty cache, so it refetches every time the parent renders.Both are the misuse
@tanstack/query/stable-query-clientalready flags.preact-query has the same
useBaseQuery/useQueriescode and could get the same change in a follow-up.✅ Checklist
pnpm run test:pr, or these tests do not apply to this pull request. (I rantest:lib,test:types,test:oxlintandbuildfor the touched packages, plusoxfmt. Thetest:buildtargets runnpm pack, and I don't have npm installed locally.)🚀 Release Impact
Summary by CodeRabbit
useQueryanduseQueriesnow correctly switch to a new query client when the active provider or explicitly supplied client changes.