Skip to content

Commit 564a4f6

Browse files
joseph0926TkDodo
joseph0926
and
TkDodo
authored
fix(query-core): ensure combine re-executes after cache restoration with memoized combine (#9592)
* fix(query-core): ensure combine re-executes after cache restoration with memoized combine * fix(query-core): add error field to result change detection * test(query-core): add test for QueriesObserver early return notification * test(query-core): improve test for QueriesObserver early return notification * refactor(query-core): use shallowEqualObjects for result comparison * refactor(query-core): improve setQueries flow and fix persist/restore issue with memoized combine * refactor(query-core): improve setQueries flow and remove log * test(react-query): update useQueries expectations for metadata change detection * test(react-query): fix test comments * test(react-query): add test for stable combine optimization on unrelated re-renders --------- Co-authored-by: Dominik Dorfmeister <office@dorfmeister.cc>
1 parent 5221029 commit 564a4f6

File tree

4 files changed

+292
-20
lines changed

4 files changed

+292
-20
lines changed

‎packages/query-core/src/__tests__/queriesObserver.test.tsx‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -347,4 +347,51 @@ describe('queriesObserver', () => {
347347
expect(queryFn1).toHaveBeenCalledTimes(1)
348348
expect(queryFn2).toHaveBeenCalledTimes(1)
349349
})
350+
351+
test('should notify when results change during early return', async () => {
352+
const key1 = queryKey()
353+
const key2 = queryKey()
354+
const queryFn1 = vi.fn().mockReturnValue(1)
355+
const queryFn2 = vi.fn().mockReturnValue(2)
356+
357+
queryClient.setQueryData(key1, 1)
358+
queryClient.setQueryData(key2, 2)
359+
360+
const observer = new QueriesObserver(queryClient, [
361+
{ queryKey: key1, queryFn: queryFn1 },
362+
{ queryKey: key2, queryFn: queryFn2 },
363+
])
364+
365+
const results: Array<Array<QueryObserverResult>> = []
366+
results.push(observer.getCurrentResult())
367+
368+
const onUpdate = vi.fn((result: Array<QueryObserverResult>) => {
369+
results.push(result)
370+
})
371+
const unsubscribe = observer.subscribe(onUpdate)
372+
const baseline = results.length
373+
374+
observer.setQueries([
375+
{
376+
queryKey: key1,
377+
queryFn: queryFn1,
378+
select: (d: any) => d + 100,
379+
},
380+
{
381+
queryKey: key2,
382+
queryFn: queryFn2,
383+
select: (d: any) => d + 100,
384+
},
385+
])
386+
387+
await vi.advanceTimersByTimeAsync(0)
388+
389+
unsubscribe()
390+
391+
expect(results.length).toBeGreaterThan(baseline)
392+
expect(results[results.length - 1]).toMatchObject([
393+
{ status: 'success', data: 101 },
394+
{ status: 'success', data: 102 },
395+
])
396+
})
350397
})

‎packages/query-core/src/queriesObserver.ts‎

Lines changed: 23 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { notifyManager } from './notifyManager'
22
import { QueryObserver } from './queryObserver'
33
import { Subscribable } from './subscribable'
4-
import { replaceEqualDeep } from './utils'
4+
import { replaceEqualDeep, shallowEqualObjects } from './utils'
55
import type {
66
DefaultedQueryObserverOptions,
77
QueryObserverOptions,
@@ -122,26 +122,34 @@ export class QueriesObserver<
122122
(observer, index) => observer !== prevObservers[index],
123123
)
124124

125-
if (prevObservers.length === newObservers.length && !hasIndexChange) {
126-
return
127-
}
125+
const hasResultChange =
126+
prevObservers.length === newObservers.length && !hasIndexChange
127+
? newResult.some((result, index) => {
128+
const prev = this.#result[index]
129+
return !prev || !shallowEqualObjects(result, prev)
130+
})
131+
: true
128132

129-
this.#observers = newObservers
130-
this.#result = newResult
133+
if (!hasIndexChange && !hasResultChange) return
131134

132-
if (!this.hasListeners()) {
133-
return
135+
if (hasIndexChange) {
136+
this.#observers = newObservers
134137
}
135138

136-
difference(prevObservers, newObservers).forEach((observer) => {
137-
observer.destroy()
138-
})
139+
this.#result = newResult
139140

140-
difference(newObservers, prevObservers).forEach((observer) => {
141-
observer.subscribe((result) => {
142-
this.#onUpdate(observer, result)
141+
if (!this.hasListeners()) return
142+
143+
if (hasIndexChange) {
144+
difference(prevObservers, newObservers).forEach((observer) => {
145+
observer.destroy()
143146
})
144-
})
147+
difference(newObservers, prevObservers).forEach((observer) => {
148+
observer.subscribe((result) => {
149+
this.#onUpdate(observer, result)
150+
})
151+
})
152+
}
145153

146154
this.#notify()
147155
})

‎packages/react-query-persist-client/src/__tests__/use-queries-with-persist.test.tsx‎

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
import { afterEach, beforeEach, describe, expect, it } from 'vitest'
2+
import { render, waitFor } from '@testing-library/react'
3+
import * as React from 'react'
4+
import { QueryClient, useQueries } from '@tanstack/react-query'
5+
import { PersistQueryClientProvider } from '@tanstack/react-query-persist-client'
6+
import type {
7+
PersistedClient,
8+
Persister,
9+
} from '@tanstack/query-persist-client-core'
10+
import type { QueryObserverResult } from '@tanstack/react-query'
11+
12+
describe('useQueries with persist and memoized combine', () => {
13+
const storage: { [key: string]: string } = {}
14+
15+
beforeEach(() => {
16+
Object.defineProperty(window, 'localStorage', {
17+
value: {
18+
getItem: (key: string) => storage[key] || null,
19+
setItem: (key: string, value: string) => {
20+
storage[key] = value
21+
},
22+
removeItem: (key: string) => {
23+
delete storage[key]
24+
},
25+
clear: () => {
26+
Object.keys(storage).forEach((key) => delete storage[key])
27+
},
28+
},
29+
writable: true,
30+
})
31+
})
32+
33+
afterEach(() => {
34+
Object.keys(storage).forEach((key) => delete storage[key])
35+
})
36+
37+
it('should update UI when combine is memoized with persist', async () => {
38+
const queryClient = new QueryClient({
39+
defaultOptions: {
40+
queries: {
41+
staleTime: 30_000,
42+
gcTime: 1000 * 60 * 60 * 24,
43+
},
44+
},
45+
})
46+
47+
const persister: Persister = {
48+
persistClient: (client: PersistedClient) => {
49+
storage['REACT_QUERY_OFFLINE_CACHE'] = JSON.stringify(client)
50+
return Promise.resolve()
51+
},
52+
restoreClient: async () => {
53+
const stored = storage['REACT_QUERY_OFFLINE_CACHE']
54+
if (stored) {
55+
await new Promise((resolve) => setTimeout(resolve, 10))
56+
return JSON.parse(stored) as PersistedClient
57+
}
58+
return undefined
59+
},
60+
removeClient: () => {
61+
delete storage['REACT_QUERY_OFFLINE_CACHE']
62+
return Promise.resolve()
63+
},
64+
}
65+
66+
const persistedData: PersistedClient = {
67+
timestamp: Date.now(),
68+
buster: '',
69+
clientState: {
70+
mutations: [],
71+
queries: [1, 2, 3].map((id) => ({
72+
queryHash: `["post",${id}]`,
73+
queryKey: ['post', id],
74+
state: {
75+
data: id,
76+
dataUpdateCount: 1,
77+
dataUpdatedAt: Date.now() - 1000,
78+
error: null,
79+
errorUpdateCount: 0,
80+
errorUpdatedAt: 0,
81+
fetchFailureCount: 0,
82+
fetchFailureReason: null,
83+
fetchMeta: null,
84+
isInvalidated: false,
85+
status: 'success' as const,
86+
fetchStatus: 'idle' as const,
87+
},
88+
})),
89+
},
90+
}
91+
92+
storage['REACT_QUERY_OFFLINE_CACHE'] = JSON.stringify(persistedData)
93+
94+
function TestComponent() {
95+
const combinedQueries = useQueries({
96+
queries: [1, 2, 3].map((id) => ({
97+
queryKey: ['post', id],
98+
queryFn: () => Promise.resolve(id),
99+
staleTime: 30_000,
100+
})),
101+
combine: React.useCallback(
102+
(results: Array<QueryObserverResult<number, Error>>) => ({
103+
data: results.map((r) => r.data),
104+
isPending: results.some((r) => r.isPending),
105+
}),
106+
[],
107+
),
108+
})
109+
110+
return (
111+
<div>
112+
<div data-testid="pending">{String(combinedQueries.isPending)}</div>
113+
<div data-testid="data">
114+
{combinedQueries.data.filter((d) => d !== undefined).join(',')}
115+
</div>
116+
</div>
117+
)
118+
}
119+
120+
const { getByTestId } = render(
121+
<PersistQueryClientProvider
122+
client={queryClient}
123+
persistOptions={{ persister }}
124+
>
125+
<TestComponent />
126+
</PersistQueryClientProvider>,
127+
)
128+
129+
await waitFor(() => {
130+
expect(getByTestId('pending').textContent).toBe('false')
131+
expect(getByTestId('data').textContent).toBe('1,2,3')
132+
})
133+
})
134+
})

‎packages/react-query/src/__tests__/useQueries.test.tsx‎

Lines changed: 88 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1210,7 +1210,7 @@ describe('useQueries', () => {
12101210

12111211
const length = results.length
12121212

1213-
expect([4, 5]).toContain(results.length)
1213+
expect([4, 5, 6]).toContain(results.length)
12141214

12151215
expect(results[results.length - 1]).toStrictEqual({
12161216
combined: true,
@@ -1379,8 +1379,8 @@ describe('useQueries', () => {
13791379

13801380
fireEvent.click(rendered.getByRole('button', { name: /rerender/i }))
13811381

1382-
// no increase because just a re-render
1383-
expect(spy).toHaveBeenCalledTimes(3)
1382+
// one extra call due to recomputing the combined result on rerender
1383+
expect(spy).toHaveBeenCalledTimes(4)
13841384

13851385
value = 1
13861386

@@ -1391,8 +1391,8 @@ describe('useQueries', () => {
13911391
rendered.getByText('data: true first result:1,second result:1'),
13921392
).toBeInTheDocument()
13931393

1394-
// two value changes = two re-renders
1395-
expect(spy).toHaveBeenCalledTimes(5)
1394+
// refetch with new values triggers: both pending -> one pending -> both resolved
1395+
expect(spy).toHaveBeenCalledTimes(7)
13961396
})
13971397

13981398
it('should re-run combine if the functional reference changes', async () => {
@@ -1658,4 +1658,87 @@ describe('useQueries', () => {
16581658
),
16591659
).toBeInTheDocument()
16601660
})
1661+
1662+
it('should not re-run stable combine on unrelated re-render', async () => {
1663+
const key1 = queryKey()
1664+
const key2 = queryKey()
1665+
1666+
const client = new QueryClient()
1667+
1668+
const spy = vi.fn()
1669+
1670+
function Page() {
1671+
const [unrelatedState, setUnrelatedState] = React.useState(0)
1672+
1673+
const queries = useQueries(
1674+
{
1675+
queries: [
1676+
{
1677+
queryKey: key1,
1678+
queryFn: async () => {
1679+
await sleep(10)
1680+
return 'first result'
1681+
},
1682+
},
1683+
{
1684+
queryKey: key2,
1685+
queryFn: async () => {
1686+
await sleep(20)
1687+
return 'second result'
1688+
},
1689+
},
1690+
],
1691+
combine: React.useCallback((results: Array<QueryObserverResult>) => {
1692+
const result = {
1693+
combined: true,
1694+
res: results.map((res) => res.data).join(','),
1695+
}
1696+
spy(result)
1697+
return result
1698+
}, []),
1699+
},
1700+
client,
1701+
)
1702+
1703+
return (
1704+
<div>
1705+
<div>
1706+
data: {String(queries.combined)} {queries.res}
1707+
</div>
1708+
<div>unrelated: {unrelatedState}</div>
1709+
<button onClick={() => setUnrelatedState((s) => s + 1)}>
1710+
increment
1711+
</button>
1712+
</div>
1713+
)
1714+
}
1715+
1716+
const rendered = render(<Page />)
1717+
1718+
await vi.advanceTimersByTimeAsync(21)
1719+
expect(
1720+
rendered.getByText('data: true first result,second result'),
1721+
).toBeInTheDocument()
1722+
1723+
// initial renders: both pending, one pending, both resolved
1724+
expect(spy).toHaveBeenCalledTimes(3)
1725+
1726+
fireEvent.click(rendered.getByRole('button', { name: /increment/i }))
1727+
1728+
await vi.advanceTimersByTimeAsync(0)
1729+
1730+
expect(rendered.getByText('unrelated: 1')).toBeInTheDocument()
1731+
1732+
// combine should NOT re-run for unrelated re-render with stable reference
1733+
expect(spy).toHaveBeenCalledTimes(3)
1734+
1735+
fireEvent.click(rendered.getByRole('button', { name: /increment/i }))
1736+
1737+
await vi.advanceTimersByTimeAsync(0)
1738+
1739+
expect(rendered.getByText('unrelated: 2')).toBeInTheDocument()
1740+
1741+
// still no extra calls to combine
1742+
expect(spy).toHaveBeenCalledTimes(3)
1743+
})
16611744
})

0 commit comments

Comments
 (0)