Skip to content

Commit 4096064

Browse files
jvuotijvuoti
committed
fix(useQueries): search for unused observers if keepPreviousData is on (#2680)
When keepPreviousData is on, search for a query observer no longer used instead of relying on observer order and indexing. Trying to match observers by index could cause infinite request loops when switching queries.
1 parent 8013d4a commit 4096064

File tree

3 files changed

+247
-58
lines changed

3 files changed

+247
-58
lines changed

‎src/core/queriesObserver.ts‎

Lines changed: 101 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { difference, replaceAt } from './utils'
1+
import { difference, notNullOrUndefined, replaceAt } from './utils'
22
import { notifyManager } from './notifyManager'
33
import type { QueryObserverOptions, QueryObserverResult } from './types'
44
import type { QueryClient } from './queryClient'
@@ -64,78 +64,120 @@ export class QueriesObserver extends Subscribable<QueriesObserverListener> {
6464
}
6565

6666
getOptimisticResult(queries: QueryObserverOptions[]): QueryObserverResult[] {
67-
return queries.map((options, index) => {
68-
const defaultedOptions = this.client.defaultQueryObserverOptions(options)
69-
return this.getObserver(defaultedOptions, index).getOptimisticResult(
70-
defaultedOptions
71-
)
67+
return this.getUpdatedObservers(queries).newResult
68+
}
69+
70+
private getUpdatedObservers(
71+
queries: QueryObserverOptions[],
72+
notifyOptions?: NotifyOptions
73+
): QueryObserverUpdate {
74+
const prevObservers = this.observers
75+
const prevObserversMap = this.observersMap
76+
const newResult: QueryObserverResult[] = []
77+
const newObservers: QueryObserver[] = []
78+
const newObserversMap: Record<string, QueryObserver> = {}
79+
80+
const defaultedQueryOptions = queries.map(o =>
81+
this.client.defaultQueryObserverOptions(o)
82+
)
83+
const matchingObservers = defaultedQueryOptions
84+
.map(o => {
85+
if (o.queryHash == null) {
86+
return null
87+
}
88+
const match = prevObserversMap[o.queryHash]
89+
if (match != null) {
90+
match.setOptions(o, notifyOptions)
91+
return match
92+
}
93+
return null
94+
})
95+
.filter(notNullOrUndefined)
96+
97+
const matchedQueryHashes = matchingObservers
98+
.map(o => o?.options.queryHash)
99+
.filter(notNullOrUndefined)
100+
const unmatchedQueries = defaultedQueryOptions.filter(
101+
o =>
102+
o.queryHash !== undefined && !matchedQueryHashes.includes(o.queryHash)
103+
)
104+
105+
const unmatchedObservers = prevObservers.filter(
106+
p => !matchingObservers.includes(p)
107+
)
108+
109+
const newlyMatchedOrCreatedObservers = unmatchedQueries.map(q => {
110+
if (q.keepPreviousData && unmatchedObservers.length > 0) {
111+
// use the first existing but no longer matched query to keep query data for any new queries
112+
const firstObserver = unmatchedObservers.splice(0, 1)[0]
113+
if (firstObserver !== undefined) {
114+
firstObserver.setOptions(q, notifyOptions)
115+
return firstObserver
116+
}
117+
}
118+
return this.getObserver(q)
72119
})
120+
121+
matchingObservers
122+
.concat(newlyMatchedOrCreatedObservers)
123+
.forEach(observer => {
124+
newObservers.push(observer)
125+
newResult.push(observer.getCurrentResult())
126+
newObserversMap[observer.options.queryHash!] = observer
127+
})
128+
129+
return {
130+
newResult: newResult,
131+
newObservers: newObservers,
132+
newObserversMap: newObserversMap,
133+
}
73134
}
74135

75-
private getObserver(
76-
options: QueryObserverOptions,
77-
index: number
78-
): QueryObserver {
136+
private getObserver(options: QueryObserverOptions): QueryObserver {
79137
const defaultedOptions = this.client.defaultQueryObserverOptions(options)
80-
let currentObserver = this.observersMap[defaultedOptions.queryHash!]
81-
if (!currentObserver && defaultedOptions.keepPreviousData) {
82-
currentObserver = this.observers[index]
83-
}
138+
const currentObserver = this.observersMap[defaultedOptions.queryHash!]
84139
return currentObserver ?? new QueryObserver(this.client, defaultedOptions)
85140
}
86141

87142
private updateObservers(notifyOptions?: NotifyOptions): void {
88143
notifyManager.batch(() => {
89-
let hasIndexChange = false
90-
91144
const prevObservers = this.observers
92-
const prevObserversMap = this.observersMap
93-
94-
const newResult: QueryObserverResult[] = []
95-
const newObservers: QueryObserver[] = []
96-
const newObserversMap: Record<string, QueryObserver> = {}
97-
98-
this.queries.forEach((options, i) => {
99-
const defaultedOptions = this.client.defaultQueryObserverOptions(
100-
options
101-
)
102-
const queryHash = defaultedOptions.queryHash!
103-
const observer = this.getObserver(defaultedOptions, i)
104-
105-
if (prevObserversMap[queryHash] || defaultedOptions.keepPreviousData) {
106-
observer.setOptions(defaultedOptions, notifyOptions)
107-
}
108-
109-
if (observer !== prevObservers[i]) {
110-
hasIndexChange = true
111-
}
112-
113-
newObservers.push(observer)
114-
newResult.push(observer.getCurrentResult())
115-
newObserversMap[queryHash] = observer
116-
})
145+
const updatedObservers = this.getUpdatedObservers(
146+
this.queries,
147+
notifyOptions
148+
)
117149

118-
if (prevObservers.length === newObservers.length && !hasIndexChange) {
150+
const hasIndexChange = updatedObservers.newObservers.some(
151+
(o, i) => o !== prevObservers[i]
152+
)
153+
if (
154+
prevObservers.length === updatedObservers.newObservers.length &&
155+
!hasIndexChange
156+
) {
119157
return
120158
}
121159

122-
this.observers = newObservers
123-
this.observersMap = newObserversMap
124-
this.result = newResult
160+
this.observers = updatedObservers.newObservers
161+
this.observersMap = updatedObservers.newObserversMap
162+
this.result = updatedObservers.newResult
125163

126164
if (!this.hasListeners()) {
127165
return
128166
}
129167

130-
difference(prevObservers, newObservers).forEach(observer => {
131-
observer.destroy()
132-
})
168+
difference(prevObservers, updatedObservers.newObservers).forEach(
169+
observer => {
170+
observer.destroy()
171+
}
172+
)
133173

134-
difference(newObservers, prevObservers).forEach(observer => {
135-
observer.subscribe(result => {
136-
this.onUpdate(observer, result)
137-
})
138-
})
174+
difference(updatedObservers.newObservers, prevObservers).forEach(
175+
observer => {
176+
observer.subscribe(result => {
177+
this.onUpdate(observer, result)
178+
})
179+
}
180+
)
139181

140182
this.notify()
141183
})
@@ -157,3 +199,9 @@ export class QueriesObserver extends Subscribable<QueriesObserverListener> {
157199
})
158200
}
159201
}
202+
203+
type QueryObserverUpdate = {
204+
newResult: QueryObserverResult[]
205+
newObservers: QueryObserver[]
206+
newObserversMap: Record<string, QueryObserver>
207+
}

‎src/core/utils.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -453,3 +453,10 @@ export function getAbortController(): AbortController | undefined {
453453
return new AbortController()
454454
}
455455
}
456+
457+
/**
458+
* Type predicate to filter an array of null or undefined elements
459+
*/
460+
export function notNullOrUndefined<T>(value: T | null | undefined): value is T {
461+
return value !== null && value !== undefined;
462+
}

‎src/react/tests/useQueries.test.tsx‎

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

9999
renderWithClient(queryClient, <Page />)
100100

101-
await waitFor(() => expect(states.length).toBe(7))
101+
await waitFor(() => expect(states.length).toBe(8))
102102

103103
expect(states[0]).toMatchObject([
104104
{
@@ -136,10 +136,14 @@ describe('useQueries', () => {
136136
{ status: 'success', data: 5, isPreviousData: true, isFetching: true },
137137
])
138138
expect(states[5]).toMatchObject([
139-
{ status: 'success', data: 4, isPreviousData: false, isFetching: false },
139+
{ status: 'success', data: 2, isPreviousData: true, isFetching: true },
140140
{ status: 'success', data: 5, isPreviousData: true, isFetching: true },
141141
])
142142
expect(states[6]).toMatchObject([
143+
{ status: 'success', data: 4, isPreviousData: false, isFetching: false },
144+
{ status: 'success', data: 5, isPreviousData: true, isFetching: true },
145+
])
146+
expect(states[7]).toMatchObject([
143147
{ status: 'success', data: 4, isPreviousData: false, isFetching: false },
144148
{ status: 'success', data: 10, isPreviousData: false, isFetching: false },
145149
])
@@ -175,7 +179,7 @@ describe('useQueries', () => {
175179

176180
renderWithClient(queryClient, <Page />)
177181

178-
await waitFor(() => expect(states.length).toBe(8))
182+
await waitFor(() => expect(states.length).toBe(10))
179183

180184
expect(states[0]).toMatchObject([
181185
{
@@ -226,7 +230,7 @@ describe('useQueries', () => {
226230
},
227231
])
228232
expect(states[5]).toMatchObject([
229-
{ status: 'success', data: 6, isPreviousData: false, isFetching: false },
233+
{ status: 'success', data: 4, isPreviousData: true, isFetching: true },
230234
{ status: 'success', data: 8, isPreviousData: true, isFetching: true },
231235
{
232236
status: 'loading',
@@ -236,6 +240,26 @@ describe('useQueries', () => {
236240
},
237241
])
238242
expect(states[6]).toMatchObject([
243+
{ status: 'success', data: 4, isPreviousData: true, isFetching: true },
244+
{ status: 'success', data: 8, isPreviousData: true, isFetching: true },
245+
{
246+
status: 'loading',
247+
data: undefined,
248+
isPreviousData: false,
249+
isFetching: true,
250+
},
251+
])
252+
expect(states[7]).toMatchObject([
253+
{ status: 'success', data: 6, isPreviousData: false, isFetching: false },
254+
{ status: 'success', data: 8, isPreviousData: true, isFetching: true },
255+
{
256+
status: 'loading',
257+
data: undefined,
258+
isPreviousData: false,
259+
isFetching: true,
260+
},
261+
])
262+
expect(states[8]).toMatchObject([
239263
{ status: 'success', data: 6, isPreviousData: false, isFetching: false },
240264
{ status: 'success', data: 12, isPreviousData: false, isFetching: false },
241265
{
@@ -245,13 +269,123 @@ describe('useQueries', () => {
245269
isFetching: true,
246270
},
247271
])
248-
expect(states[7]).toMatchObject([
272+
expect(states[9]).toMatchObject([
249273
{ status: 'success', data: 6, isPreviousData: false, isFetching: false },
250274
{ status: 'success', data: 12, isPreviousData: false, isFetching: false },
251275
{ status: 'success', data: 18, isPreviousData: false, isFetching: false },
252276
])
253277
})
254278

279+
it('should keep previous data when switching between queries', async () => {
280+
const key = queryKey()
281+
const states: UseQueryResult[][] = []
282+
283+
function Page() {
284+
const [series1, setSeries1] = React.useState(1)
285+
const [series2, setSeries2] = React.useState(2)
286+
const ids = [series1, series2]
287+
288+
const result = useQueries(
289+
ids.map(id => {
290+
return {
291+
queryKey: [key, id],
292+
queryFn: async () => {
293+
await sleep(5)
294+
return id * 5
295+
},
296+
keepPreviousData: true,
297+
}
298+
})
299+
)
300+
301+
states.push(result)
302+
303+
React.useEffect(() => {
304+
setActTimeout(() => {
305+
setSeries2(3)
306+
}, 20)
307+
}, [])
308+
309+
React.useEffect(() => {
310+
setActTimeout(() => {
311+
setSeries1(2)
312+
}, 50)
313+
}, [])
314+
315+
return null
316+
}
317+
318+
renderWithClient(queryClient, <Page />)
319+
320+
await waitFor(() => expect(states.length).toBe(12))
321+
322+
console.log('states: ' + JSON.stringify(states))
323+
324+
expect(states[0]).toMatchObject([
325+
{
326+
status: 'loading',
327+
data: undefined,
328+
isPreviousData: false,
329+
isFetching: true,
330+
},
331+
{
332+
status: 'loading',
333+
data: undefined,
334+
isPreviousData: false,
335+
isFetching: true,
336+
},
337+
])
338+
expect(states[1]).toMatchObject([
339+
{ status: 'success', data: 5, isPreviousData: false, isFetching: false },
340+
{
341+
status: 'loading',
342+
data: undefined,
343+
isPreviousData: false,
344+
isFetching: true,
345+
},
346+
])
347+
expect(states[2]).toMatchObject([
348+
{ status: 'success', data: 5, isPreviousData: false, isFetching: false },
349+
{ status: 'success', data: 10, isPreviousData: false, isFetching: false },
350+
])
351+
expect(states[3]).toMatchObject([
352+
{ status: 'success', data: 5, isPreviousData: false, isFetching: false },
353+
{ status: 'success', data: 10, isPreviousData: true, isFetching: true },
354+
])
355+
expect(states[4]).toMatchObject([
356+
{ status: 'success', data: 5, isPreviousData: false, isFetching: false },
357+
{ status: 'success', data: 10, isPreviousData: true, isFetching: true },
358+
])
359+
expect(states[5]).toMatchObject([
360+
{ status: 'success', data: 5, isPreviousData: false, isFetching: false },
361+
{ status: 'success', data: 15, isPreviousData: false, isFetching: false },
362+
])
363+
expect(states[6]).toMatchObject([
364+
{ status: 'success', data: 10, isPreviousData: false, isFetching: true },
365+
{ status: 'success', data: 15, isPreviousData: false, isFetching: true },
366+
])
367+
expect(states[7]).toMatchObject([
368+
{ status: 'success', data: 10, isPreviousData: false, isFetching: true },
369+
{ status: 'success', data: 15, isPreviousData: false, isFetching: true },
370+
])
371+
expect(states[8]).toMatchObject([
372+
{ status: 'success', data: 10, isPreviousData: false, isFetching: true },
373+
{ status: 'success', data: 15, isPreviousData: false, isFetching: true },
374+
])
375+
expect(states[9]).toMatchObject([
376+
{ status: 'success', data: 10, isPreviousData: false, isFetching: true },
377+
{ status: 'success', data: 15, isPreviousData: false, isFetching: true },
378+
])
379+
expect(states[10]).toMatchObject([
380+
{ status: 'success', data: 10, isPreviousData: false, isFetching: false },
381+
{ status: 'success', data: 15, isPreviousData: false, isFetching: true },
382+
])
383+
expect(states[11]).toMatchObject([
384+
{ status: 'success', data: 10, isPreviousData: false, isFetching: false },
385+
{ status: 'success', data: 15, isPreviousData: false, isFetching: false },
386+
])
387+
})
388+
255389
it('handles type parameter - tuple of tuples', async () => {
256390
const key1 = queryKey()
257391
const key2 = queryKey()

0 commit comments

Comments
 (0)