-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
fix(react-query/HydrationBoundary): prevent unnecessary refetch during hydration #9968
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 16 commits
8626d42
25499fc
695fa28
2738c99
9267ccc
992bee3
b8f5e0a
9d64cf1
bcd214e
607f405
aa4c923
653df2c
08715e6
d3bf064
2a65e27
78ce3f5
058fee0
cdbc754
f840a0a
bb9cced
b63f6ec
c4ad620
888f65a
555e2b9
5fec087
8da1001
a8d3c99
2faa447
2362ae8
c37ad5e
95d9f86
e7e6882
dba4b76
e6c8ad8
e4c589f
8ab4e34
8c1d38f
71806f7
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 |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| '@tanstack/query-core': patch | ||
| '@tanstack/react-query': patch | ||
| --- | ||
|
|
||
| fix(react-query/HydrationBoundary): prevent unnecessary refetch during hydration |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,11 @@ import type { QueryClient } from './queryClient' | |
| import type { Query, QueryState } from './query' | ||
| import type { Mutation, MutationState } from './mutation' | ||
|
|
||
| // WeakSet to track queries that are pending hydration | ||
| // Used to prevent double-fetching when HydrationBoundary defers hydration to useEffect | ||
| export const pendingHydrationQueries: WeakSet<Query<any, any, any, any>> = | ||
|
Collaborator
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. I don't think we can use a global singleton to keep track of this, what if there are several Annoying as that is, I think we need to use a context for this. Note that there is some prior art with |
||
| new WeakSet() | ||
|
|
||
| // TYPES | ||
| type TransformerFn = (data: any) => any | ||
| function defaultTransformerFn(data: any): any { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import { focusManager } from './focusManager' | ||
| import { pendingHydrationQueries } from './hydration' | ||
| import { notifyManager } from './notifyManager' | ||
| import { fetchState } from './query' | ||
| import { Subscribable } from './subscribable' | ||
|
|
@@ -96,7 +97,24 @@ export class QueryObserver< | |
| if (this.listeners.size === 1) { | ||
| this.#currentQuery.addObserver(this) | ||
|
|
||
| if (shouldFetchOnMount(this.#currentQuery, this.options)) { | ||
| // Check if this query is pending hydration | ||
| // If so, skip fetch unless refetchOnMount is explicitly 'always' | ||
| const hasPendingHydration = pendingHydrationQueries.has( | ||
| this.#currentQuery, | ||
| ) | ||
|
|
||
| const resolvedRefetchOnMount = | ||
| typeof this.options.refetchOnMount === 'function' | ||
| ? this.options.refetchOnMount(this.#currentQuery) | ||
| : this.options.refetchOnMount | ||
|
|
||
| const shouldSkipFetchForHydration = | ||
| hasPendingHydration && resolvedRefetchOnMount !== 'always' | ||
|
|
||
| if ( | ||
| shouldFetchOnMount(this.#currentQuery, this.options) && | ||
| !shouldSkipFetchForHydration | ||
| ) { | ||
|
Comment on lines
+114
to
+117
Collaborator
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. There's also logic in This gets into some other tricky territory (where we might already have other bugs tbh), which is that the first render needs to match the render on the server. If the data was not stale when server rendering, but is now stale, we need to:
|
||
| this.#executeFetch() | ||
| } else { | ||
| this.updateResult() | ||
|
|
||
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.
Thank you for this PR! I've only had time to skim it so far and it will take some time before I can dig in properly, but I wanted to provide some quick feedback here.
We also need to account for the case where there is some time in between fetching on the server, and hydrating on the client. This commonly happens when the markup is cached, in which case the hydration can happen days after the fetch on the server. In this situation, we do want to render with the stale data initially (this is what the SSR pass did and we need to match it), but we also want to immediately refetch the data because it's stale.