-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
fix(query-core): fix combine function cache invalidation with stable reference #9618
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
Closed
joseph0926
wants to merge
9
commits into
TanStack:main
from
joseph0926:fix/combine-stable-reference-caching
+374
−0
Closed
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
3444783
fix(query-core): fix combine function cache invalidation with stable …
joseph0926 6387623
test(query-core): test file renaming
joseph0926 abc9c0d
docs(query-core): add clarifying comments for combine cache fix
joseph0926 c515b4c
test(query-core): add test coverage for combine cache invalidation fix
joseph0926 d8050a9
test(query-core): improve combine cache test with stricter assertions
joseph0926 f1eb0a4
Merge branch 'main' into fix/combine-stable-reference-caching
joseph0926 dc892fa
Merge branch 'main' into fix/combine-stable-reference-caching
TkDodo d0701cc
Merge branch 'main' into fix/combine-stable-reference-caching
joseph0926 02ddb7e
Merge branch 'main' into fix/combine-stable-reference-caching
joseph0926 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
319 changes: 319 additions & 0 deletions
319
packages/react-query/src/__tests__/useQueries-combine.test.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,319 @@ | ||
import { describe, expect, test } from 'vitest' | ||
import { renderHook, waitFor } from '@testing-library/react' | ||
import React from 'react' | ||
import { | ||
QueryClient, | ||
QueryClientProvider, | ||
useQueries, | ||
} from '@tanstack/react-query' | ||
|
||
describe('useQueries combine memoization in React', () => { | ||
test('stable reference combine should update immediately when queries change', () => { | ||
const queryClient = new QueryClient({ | ||
defaultOptions: { | ||
queries: { | ||
retry: false, | ||
}, | ||
}, | ||
}) | ||
|
||
const wrapper = ({ children }: { children: React.ReactNode }) => ( | ||
<QueryClientProvider client={queryClient}>{children}</QueryClientProvider> | ||
) | ||
|
||
const stableCombine = (results: any) => results | ||
|
||
const { result, rerender } = renderHook( | ||
({ n }: { n: number }) => { | ||
const queries = useQueries({ | ||
queries: [...Array(n).keys()].map((i) => ({ | ||
queryKey: ['stable', i], | ||
queryFn: () => Promise.resolve(i + 100), | ||
})), | ||
combine: stableCombine, | ||
}) | ||
|
||
return queries | ||
}, | ||
{ | ||
wrapper, | ||
initialProps: { n: 0 }, | ||
}, | ||
) | ||
|
||
expect(result.current.length).toBe(0) | ||
|
||
rerender({ n: 1 }) | ||
expect(result.current.length).toBe(1) | ||
|
||
rerender({ n: 2 }) | ||
expect(result.current.length).toBe(2) | ||
}) | ||
|
||
test('inline combine should update immediately when queries change', () => { | ||
const queryClient = new QueryClient({ | ||
defaultOptions: { | ||
queries: { | ||
retry: false, | ||
}, | ||
}, | ||
}) | ||
|
||
const wrapper = ({ children }: { children: React.ReactNode }) => ( | ||
<QueryClientProvider client={queryClient}>{children}</QueryClientProvider> | ||
) | ||
|
||
const { result, rerender } = renderHook( | ||
({ n }: { n: number }) => { | ||
const queries = useQueries({ | ||
queries: [...Array(n).keys()].map((i) => ({ | ||
queryKey: ['inline', i], | ||
queryFn: () => Promise.resolve(i + 100), | ||
})), | ||
combine: (results) => results, | ||
}) | ||
|
||
return queries | ||
}, | ||
{ | ||
wrapper, | ||
initialProps: { n: 0 }, | ||
}, | ||
) | ||
|
||
expect(result.current.length).toBe(0) | ||
|
||
rerender({ n: 1 }) | ||
expect(result.current.length).toBe(1) | ||
|
||
rerender({ n: 2 }) | ||
expect(result.current.length).toBe(2) | ||
}) | ||
}) | ||
|
||
describe('useQueries combine memoization edge cases', () => { | ||
test('should handle dynamic query array changes correctly', () => { | ||
const queryClient = new QueryClient({ | ||
defaultOptions: { | ||
queries: { | ||
retry: false, | ||
}, | ||
}, | ||
}) | ||
|
||
const wrapper = ({ children }: { children: React.ReactNode }) => ( | ||
<QueryClientProvider client={queryClient}>{children}</QueryClientProvider> | ||
) | ||
|
||
const stableCombine = (results: any) => results | ||
|
||
const { result, rerender } = renderHook( | ||
({ ids }: { ids: Array<number> }) => { | ||
const queries = useQueries({ | ||
queries: ids.map((id) => ({ | ||
queryKey: ['test', id], | ||
queryFn: () => Promise.resolve(id), | ||
})), | ||
combine: stableCombine, | ||
}) | ||
return queries | ||
}, | ||
{ | ||
wrapper, | ||
initialProps: { ids: [] } as { ids: Array<number> }, | ||
}, | ||
) | ||
|
||
expect(result.current.length).toBe(0) | ||
|
||
rerender({ ids: [1, 2, 3] }) | ||
expect(result.current.length).toBe(3) | ||
|
||
rerender({ ids: [2, 3] }) | ||
expect(result.current.length).toBe(2) | ||
|
||
rerender({ ids: [2, 3, 4, 5] }) | ||
expect(result.current.length).toBe(4) | ||
|
||
rerender({ ids: [] }) | ||
expect(result.current.length).toBe(0) | ||
}) | ||
|
||
test('should handle combine function that transforms data', () => { | ||
const queryClient = new QueryClient({ | ||
defaultOptions: { | ||
queries: { | ||
retry: false, | ||
}, | ||
}, | ||
}) | ||
|
||
const wrapper = ({ children }: { children: React.ReactNode }) => ( | ||
<QueryClientProvider client={queryClient}>{children}</QueryClientProvider> | ||
) | ||
|
||
const transformCombine = (results: any) => ({ | ||
count: results.length, | ||
data: results, | ||
}) | ||
|
||
const { result, rerender } = renderHook( | ||
({ n }: { n: number }) => { | ||
const queries = useQueries({ | ||
queries: [...Array(n).keys()].map((i) => ({ | ||
queryKey: ['transform', i], | ||
queryFn: () => Promise.resolve(i), | ||
})), | ||
combine: transformCombine, | ||
}) | ||
return queries | ||
}, | ||
{ | ||
wrapper, | ||
initialProps: { n: 0 }, | ||
}, | ||
) | ||
|
||
expect(result.current.count).toBe(0) | ||
|
||
rerender({ n: 2 }) | ||
expect(result.current.count).toBe(2) | ||
expect(result.current.data.length).toBe(2) | ||
|
||
rerender({ n: 5 }) | ||
expect(result.current.count).toBe(5) | ||
expect(result.current.data.length).toBe(5) | ||
}) | ||
|
||
test('should not break when switching between stable and inline combine', () => { | ||
const queryClient = new QueryClient({ | ||
defaultOptions: { | ||
queries: { | ||
retry: false, | ||
}, | ||
}, | ||
}) | ||
|
||
const wrapper = ({ children }: { children: React.ReactNode }) => ( | ||
<QueryClientProvider client={queryClient}>{children}</QueryClientProvider> | ||
) | ||
|
||
const stableCombine = (results: any) => results | ||
|
||
const { result, rerender } = renderHook( | ||
({ useStable }: { useStable: boolean }) => { | ||
const queries = useQueries({ | ||
queries: [ | ||
{ | ||
queryKey: ['switch', 1], | ||
queryFn: () => Promise.resolve(1), | ||
}, | ||
], | ||
combine: useStable ? stableCombine : (results) => results, | ||
}) | ||
return queries | ||
}, | ||
{ | ||
wrapper, | ||
initialProps: { useStable: true }, | ||
}, | ||
) | ||
|
||
expect(result.current.length).toBe(1) | ||
|
||
rerender({ useStable: false }) | ||
expect(result.current.length).toBe(1) | ||
|
||
rerender({ useStable: true }) | ||
expect(result.current.length).toBe(1) | ||
}) | ||
|
||
test('should handle same length but different queries', async () => { | ||
const queryClient = new QueryClient({ | ||
defaultOptions: { | ||
queries: { | ||
retry: false, | ||
}, | ||
}, | ||
}) | ||
|
||
const wrapper = ({ children }: { children: React.ReactNode }) => ( | ||
<QueryClientProvider client={queryClient}>{children}</QueryClientProvider> | ||
) | ||
|
||
const stableCombine = (results: any) => results | ||
|
||
const { result, rerender } = renderHook( | ||
({ keys }: { keys: Array<string> }) => { | ||
const queries = useQueries({ | ||
queries: keys.map((key) => ({ | ||
queryKey: [key], | ||
queryFn: () => Promise.resolve(key), | ||
})), | ||
combine: stableCombine, | ||
}) | ||
return queries | ||
}, | ||
{ | ||
wrapper, | ||
initialProps: { keys: ['a', 'b'] }, | ||
}, | ||
) | ||
|
||
expect(result.current.length).toBe(2) | ||
|
||
rerender({ keys: ['c', 'd'] }) | ||
expect(result.current.length).toBe(2) | ||
|
||
// Note: Same-length changes may use cached result for one render cycle, | ||
// but data will be correct after setQueries updates this.#result | ||
await waitFor(() => { | ||
expect(result.current[0]?.data).toBe('c') | ||
expect(result.current[1]?.data).toBe('d') | ||
}) | ||
}) | ||
|
||
test('should handle query order changes with same length', async () => { | ||
const queryClient = new QueryClient({ | ||
defaultOptions: { | ||
queries: { | ||
retry: false, | ||
}, | ||
}, | ||
}) | ||
|
||
const wrapper = ({ children }: { children: React.ReactNode }) => ( | ||
<QueryClientProvider client={queryClient}>{children}</QueryClientProvider> | ||
) | ||
|
||
const stableCombine = (results: any) => results | ||
|
||
const { result, rerender } = renderHook( | ||
({ keys }: { keys: Array<string> }) => { | ||
const queries = useQueries({ | ||
queries: keys.map((key) => ({ | ||
queryKey: [key], | ||
queryFn: () => Promise.resolve(key), | ||
})), | ||
combine: stableCombine, | ||
}) | ||
return queries | ||
}, | ||
{ | ||
wrapper, | ||
initialProps: { keys: ['x', 'y', 'z'] }, | ||
}, | ||
) | ||
|
||
expect(result.current.length).toBe(3) | ||
|
||
rerender({ keys: ['z', 'x', 'y'] }) | ||
expect(result.current.length).toBe(3) | ||
|
||
await waitFor(() => { | ||
expect(result.current[0]?.data).toBe('z') | ||
expect(result.current[1]?.data).toBe('x') | ||
expect(result.current[2]?.data).toBe('y') | ||
}) | ||
}) | ||
}) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I don’t think a length check is what we want here. What about situations where the length is the same, but data inside it changed?
I’m honestly not sure why
input
would change, but the check forthis.#result !== this.#lastResult
would still see things as being equal. That’s what we need to find out imoUh oh!
There was an error while loading. Please reload this page.
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.
When
getOptimisticResult
is called, a new input is created—but this happens at render time.query/packages/react-query/src/useQueries.ts
Lines 261 to 266 in 92d6b7b
On the other hand,
observer.setQueries
is executed insideuseEffect
, which means it runs at commit time.query/packages/react-query/src/useQueries.ts
Lines 281 to 286 in 92d6b7b
In other words, at the moment
getOptimisticResult
is called, the input already reflects the new value, butthis.#result
still holds the previous value.Because of this timing difference, the following sequence occurs
First render
this.#result = []
,this.#lastResult = undefined
→getOptimisticResult
is called →input = []
is created#combineResult
is called → sincethis.#lastResult = this.#result
, the result becomesthis.#result = []
,this.#lastResult = []
useEffect() { setQueries ... }
runsSecond render
4.
getOptimisticResult
is called →input = [query1]
, whilethis.#result = []
,this.#lastResult = []
5.
#combineResult
is called → stillthis.#result === this.#lastResult
([] === []
)6.
useEffect() { setQueries ... }
runs →this.#result = [query1]
For this reason, a discrepancy arises between the
result
and theinput
From a semantic perspective, it would indeed be more accurate to add a comparison such as,
input !== this.#result && input !== this.#lastInput
However, while this approach handles optimistic updates well, I believe it could also cause excessive cache misses in regular updates. That is why I chose the alternative solution of comparing lengths instead
Regarding your concern about edge cases—such as when the arrays have the same length but different order—I confirmed through the additional tests I wrote (
"should handle same length but different queries"
and"should handle query order changes with same length"
) that no issues arise there. Based on that, I decided to adopt this solution as the final choice.That said, as you pointed out, this method is ultimately more of a stopgap—it works well for optimistic updates while avoiding excessive cache misses in regular updates, but it remains a temporary measure.
Aside from this temporary workaround, I think a better solution would be to explicitly distinguish the case using a flag.
For example, something like this
Perhaps an approach along these lines?
But come to think of it, this method would still cause additional cache misses,,
For example, the expected values for the three tests below seem likely to increase from 3 to 5.
useQueries > should optimize combine if it is a stable reference
useQueries > should re-run combine if the functional reference changes
useQueries > should not re-run stable combine on unrelated re-render
So my final thought is that comparing lengths seems like the best choice when not considering overall mechanism modifications. What do you think?