Prevent useQuery from skipping hydration#10159
Prevent useQuery from skipping hydration#10159ColemanDunn wants to merge 6 commits intoTanStack:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 36edbba The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 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. 🎉 📝 WalkthroughWalkthroughHydration logic was changed so existing idle/pending queries (dataUpdatedAt === 0, status === "pending", fetchStatus === "idle") are treated like absent queries during hydration; a changeset bumps react-query and query-core as patch releases and tests added to verify no suspense-triggered refetch occurs. Changes
Sequence Diagram(s)sequenceDiagram
participant Server
participant HydrationBoundary
participant QueryClient
participant ClientComponent
Server->>QueryClient: prefetchQuery(key, data)
Server->>HydrationBoundary: provide dehydrated state
ClientComponent->>HydrationBoundary: render useQuery / useSuspenseQuery
HydrationBoundary->>QueryClient: check existingQuery state
alt existingQuery missing OR idle/pending (dataUpdatedAt==0, pending, idle)
HydrationBoundary->>QueryClient: hydrate dehydratedQuery -> replace/cache data
else existingQuery active/fetching
HydrationBoundary->>QueryClient: skip hydration for this key
end
ClientComponent->>QueryClient: read cached data (no suspense refetch)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
ColemanDunn
force-pushed
the
prevent-usequery-from-blocking-hydration
branch
from
5fedd07 to
45eb471
Compare
|
View your CI Pipeline Execution ↗ for commit 03b08ab
☁️ Nx Cloud last updated this comment at |
ColemanDunn
force-pushed
the
prevent-usequery-from-blocking-hydration
branch
from
24dbb40 to
133a6ca
Compare
ColemanDunn
force-pushed
the
prevent-usequery-from-blocking-hydration
branch
from
133a6ca to
de84710
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react-query/src/HydrationBoundary.tsx (1)
76-79: Consider merging the twonewQueries.pushbranches.Both branches push to the same array. Combining them simplifies the control flow.
♻️ Optional simplification
- if (!existingQuery) { - newQueries.push(dehydratedQuery) - } else if (existingQueryIsIdleUseQuery) { + if (!existingQuery || existingQueryIsIdleUseQuery) { newQueries.push(dehydratedQuery) } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-query/src/HydrationBoundary.tsx` around lines 76 - 79, The two branches that both call newQueries.push(dehydratedQuery) can be merged: replace the separate if (!existingQuery) { newQueries.push(dehydratedQuery) } else if (existingQueryIsIdleUseQuery) { newQueries.push(dehydratedQuery) } with a single conditional that pushes when either condition is true (e.g., if (!existingQuery || existingQueryIsIdleUseQuery) newQueries.push(dehydratedQuery)); update the logic around existingQuery and existingQueryIsIdleUseQuery so no behavior changes occur beyond the simplification.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/moody-cities-stand.md:
- Line 6: Fix the typo "hyrdation" to "hydration" in the generated changelog
text by updating the string "prevent registered useQueries from skipping
hyrdation" to "prevent registered useQueries from skipping hydration" (search
for the exact misspelled token "hyrdation" in the changelog entry and replace
it).
---
Nitpick comments:
In `@packages/react-query/src/HydrationBoundary.tsx`:
- Around line 76-79: The two branches that both call
newQueries.push(dehydratedQuery) can be merged: replace the separate if
(!existingQuery) { newQueries.push(dehydratedQuery) } else if
(existingQueryIsIdleUseQuery) { newQueries.push(dehydratedQuery) } with a single
conditional that pushes when either condition is true (e.g., if (!existingQuery
|| existingQueryIsIdleUseQuery) newQueries.push(dehydratedQuery)); update the
logic around existingQuery and existingQueryIsIdleUseQuery so no behavior
changes occur beyond the simplification.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/query-core/src/hydration.ts (1)
220-224:existingQueryIsUndefinedOrIsIdleUseQuery— naming couples implementation to a specific callerThe three-condition guard (
dataUpdatedAt === 0 && status === 'pending' && fetchStatus === 'idle') characterises a structural query state (never-fetched and idle), not the type of observer that created the entry. TheUseQuerysuffix implies caller-origin knowledge that isn't actually verified here. A name likeexistingQueryIsUndefinedOrNeverFetchedIdleorqueryIsAbsentOrIdlePendingwould be less misleading to future readers.♻️ Suggested rename
- const existingQueryIsUndefinedOrIsIdleUseQuery = + const existingQueryIsAbsentOrNeverFetchedIdle = !query || (query.state.dataUpdatedAt === 0 && query.state.status === 'pending' && query.state.fetchStatus === 'idle')And update the usage at line 270:
- (existingQueryIsUndefinedOrIsIdleUseQuery || + (existingQueryIsAbsentOrNeverFetchedIdle || (!existingQueryIsPending && !existingQueryIsFetching)) &&🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query-core/src/hydration.ts` around lines 220 - 224, Rename the misleading variable existingQueryIsUndefinedOrIsIdleUseQuery to a name that reflects the structural state check (for example existingQueryIsUndefinedOrNeverFetchedIdle or queryIsAbsentOrIdlePending) and update all references (including the use at the site that currently reads the old name) so the code checks the same three-condition guard (query is falsy OR query.state.dataUpdatedAt === 0 && query.state.status === 'pending' && query.state.fetchStatus === 'idle') but without implying a specific caller like UseQuery; update both the variable declaration and every place it is referenced (e.g., the expression currently using existingQueryIsUndefinedOrIsIdleUseQuery).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/query-core/src/hydration.ts`:
- Around line 220-224: Rename the misleading variable
existingQueryIsUndefinedOrIsIdleUseQuery to a name that reflects the structural
state check (for example existingQueryIsUndefinedOrNeverFetchedIdle or
queryIsAbsentOrIdlePending) and update all references (including the use at the
site that currently reads the old name) so the code checks the same
three-condition guard (query is falsy OR query.state.dataUpdatedAt === 0 &&
query.state.status === 'pending' && query.state.fetchStatus === 'idle') but
without implying a specific caller like UseQuery; update both the variable
declaration and every place it is referenced (e.g., the expression currently
using existingQueryIsUndefinedOrIsIdleUseQuery).
|
@Ephem can you take a look here please @ColemanDunn please fix the conflicts |
…om-blocking-hydration # Conflicts: # packages/query-core/src/hydration.ts # packages/react-query/src/__tests__/HydrationBoundary.test.tsx
Done! Thank you! |
|
There were some new considerations and tests I added in for the new |
Ephem
left a comment
Ephem
left a comment
There was a problem hiding this comment.
Thanks for trying to tackle this! I have to think some more about this, but I think this fix might be a bit too simplistic.
query?.state.dataUpdatedAt === 0 &&
query.state.status === 'pending' &&
query.state.fetchStatus === 'idle'This part is a heuristic for trying to determine when it is safe to hydrate, even if the query is already in the cache. Unfortunately, there is no such time. First, there are more situations than the reproduction itself that can cause this set of states, and it's hard to foresee all the possible side effects.
Second, imagine this case:
<UseQueryComponent>
<HydrationBoundary>
<Suspense>
<UseSuspenseQueryComponent>Anything in the Suspense can hydrate later than the <UseQueryComponent>. By that time, the observer in the <UseQueryComponent> will have been set up, so if we hydrate into the cache, we are now triggering an observed side effect in render, which is not safe.
The real problem here is that different subtrees should be able to see different data for a single query during hydration. This is not possible with the current approach, which is why this bug has gone unfixed for so long.
The planned fix for this is to avoid hydrating in render entirely. Instead we put the data to be hydrated on a context, meaning different subtrees can see different data during hydration, and a single subtree can always see the data that it was rendered with on the server. We then make all hooks read from that hydration context when necessary. We then hydrate in a useEffect at the end and remove the context data after hydration.
This separates UI reads during hydration from the cache, which is also a prerequisite for better concurrent rendering support, and it also fixes some other edge cases we have.
All that said, if we can find a safe quick fix for the issue at hand, I'm all for it, but I'm not certain yet this is one, and I'm not sure what it would take to feel safe since this is so complex to reason about. 😄
Note that a useQuery before the point of hydration should always be expected to sometimes start its own fetch on the client, even after a fix, imagine:
<UseQueryComponent>
<Suspense>
<UnrelatedComponentThatSuspends>
<HydrationBoundary>
<UseSuspenseQueryComponent>In this case, which might look contrived but is not uncommon in complex apps, the unrelated component makes it so the <UseQueryComponent> has time to start a fetch on the client before we even hit hydration. Still something we need to fix ofc, just thought I'd mention another footgun of the pattern. 😄
| (existingQueryIsIdleUseQuery || | ||
| // If the data was synchronously available, there is no need to set up | ||
| // a retryer and thus no reason to call fetch | ||
| (!syncData && |
There was a problem hiding this comment.
I think this logic is wrong. If existingQueryIsIdleUseQuery is true, we'll always call fetch, even if data was synchronously available. This causes a regression where the query quickly flashes the loading state when it shouldn't.
There was a problem hiding this comment.
ThanksI I overcorrected on that follow up commit after the merge. I have updated the code and added a test to check the loading state is in the correct state.
I know you mentioned it is hard to foresee the side effects, but I hope there are some tests we can add to determine if we can find a safe enough fix for this.
|
Thanks for the reply @Ephem! Very well put and makes sense. The pattern with this shape: <UseQueryComponent>
<HydrationBoundary>
<Suspense>
<UseSuspenseQueryComponent>has been something that is pretty common in projects I have worked on (think breadcrumbs that use Are there any specific tests that should be added to this PR that that can can ensure the fix is safe? Perhaps one for this case: <UseQueryComponent>
<Suspense>
<UnrelatedComponentThatSuspends>
<HydrationBoundary>
<UseSuspenseQueryComponent>Thanks! |
|
@ColemanDunn Thanks for keeping a constructive attitude in the face of my pessimism! 😅 An aside on a possible workaround: I've run into this case myself, where I feel I need a return useSyncExternalStore(
useCallback(
(onStoreChange: () => void) =>
cache.subscribe(event => {
if (
event.query.queryKey.length === queryKey.length &&
event.query.queryKey.every(
// The === check here will only work for primitives, to support complex keys one would need to deep compare
(k: unknown, i: number) => k === queryKey[i],
)
) {
onStoreChange();
}
}),
[cache, queryKey],
),
() => queryClient.getQueryData(queryKey),
() => queryClient.getQueryData(queryKey),
);Maybe that can be a workaround for you to until this is fixed? Back to the PR. Thinking about this some more, the thing I worry most about is probably the "hydrate in render after a store subscription has already been set up" case. You could likely get around that by also checking explicitly for whether the query has an active listener by using I haven't had time to think very deeply about this, no promises it makes the PR safe enough to merge as a quick fix etc, but I think it definitely helps! |
3 participants
🎯 Changes
Fixes #10145
Hydration is being skipped for useQueries that add to the cache above a
HydrationBoundarybut will actually not run or hydrate the queryClient.✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores