Skip to content

Commit cefd080

Browse files
incepterTkDodoDamianOsipiuk
authored
fix(core) : sync Observer 'current' properties when optimistic reading occurs (#5611)
* fix-5538 : sync Observer 'current' properties when optimistic reading occurs * Flip test on currentResult with placeholderData's order & add test * Fix tests using the same queryKey * remove options.placeHolderData from deciding whether to assign observer current properties, use equalitycheck on v5 * maybe fix for vue reactivity cbs * test(vue-query): test persister with useQuery * Force line terminator to \\n on codemods tests * Revert " Force line terminator to \\n on codemods tests" This reverts commit 6f0a469. --------- Co-authored-by: Dominik Dorfmeister <[email protected]> Co-authored-by: Damian Osipiuk <[email protected]>
1 parent c06e5dd commit cefd080

File tree

6 files changed

+204
-53
lines changed

6 files changed

+204
-53
lines changed

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

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,30 @@ export class QueryObserver<
227227
): QueryObserverResult<TData, TError> {
228228
const query = this.#client.getQueryCache().build(this.#client, options)
229229

230-
return this.createResult(query, options)
230+
const result = this.createResult(query, options)
231+
232+
if (shouldAssignObserverCurrentProperties(this, result)) {
233+
// this assigns the optimistic result to the current Observer
234+
// because if the query function changes, useQuery will be performing
235+
// an effect where it would fetch again.
236+
// When the fetch finishes, we perform a deep data cloning in order
237+
// to reuse objects references. This deep data clone is performed against
238+
// the `observer.currentResult.data` property
239+
// When QueryKey changes, we refresh the query and get new `optimistic`
240+
// result, while we leave the `observer.currentResult`, so when new data
241+
// arrives, it finds the old `observer.currentResult` which is related
242+
// to the old QueryKey. Which means that currentResult and selectData are
243+
// out of sync already.
244+
// To solve this, we move the cursor of the currentResult everytime
245+
// an observer reads an optimistic value.
246+
247+
// When keeping the previous data, the result doesn't change until new
248+
// data arrives.
249+
this.#currentResult = result
250+
this.#currentResultOptions = this.options
251+
this.#currentResultState = this.#currentQuery.state
252+
}
253+
return result
231254
}
232255

233256
getCurrentResult(): QueryObserverResult<TData, TError> {
@@ -719,3 +742,25 @@ function isStale(
719742
): boolean {
720743
return query.isStaleByTime(options.staleTime)
721744
}
745+
746+
// this function would decide if we will update the observer's 'current'
747+
// properties after an optimistic reading via getOptimisticResult
748+
function shouldAssignObserverCurrentProperties<
749+
TQueryFnData = unknown,
750+
TError = unknown,
751+
TData = TQueryFnData,
752+
TQueryData = TQueryFnData,
753+
TQueryKey extends QueryKey = QueryKey,
754+
>(
755+
observer: QueryObserver<TQueryFnData, TError, TData, TQueryData, TQueryKey>,
756+
optimisticResult: QueryObserverResult<TData, TError>,
757+
) {
758+
// if the newly created result isn't what the observer is holding as current,
759+
// then we'll need to update the properties as well
760+
if (observer.getCurrentResult() !== optimisticResult) {
761+
return true
762+
}
763+
764+
// basically, just keep previous properties if nothing changed
765+
return false
766+
}

‎packages/react-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -339,7 +339,7 @@ describe('PersistQueryClientProvider', () => {
339339
await waitFor(() => rendered.getByText('data: null'))
340340
await waitFor(() => rendered.getByText('data: hydrated'))
341341

342-
expect(states).toHaveLength(3)
342+
expect(states).toHaveLength(2)
343343

344344
expect(fetched).toBe(false)
345345

@@ -354,9 +354,6 @@ describe('PersistQueryClientProvider', () => {
354354
fetchStatus: 'idle',
355355
data: 'hydrated',
356356
})
357-
358-
// #5443 seems like we get an extra render now ...
359-
expect(states[1]).toStrictEqual(states[2])
360357
})
361358

362359
test('should call onSuccess after successful restoring', async () => {

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

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,7 @@ describe('useInfiniteQuery', () => {
213213

214214
await waitFor(() => rendered.getByText('data: 0-asc'))
215215
await waitFor(() => rendered.getByText('isFetching: false'))
216-
await waitFor(() => expect(states.length).toBe(7))
216+
await waitFor(() => expect(states.length).toBe(6))
217217

218218
expect(states[0]).toMatchObject({
219219
data: undefined,
@@ -251,15 +251,7 @@ describe('useInfiniteQuery', () => {
251251
isSuccess: true,
252252
isPlaceholderData: true,
253253
})
254-
// Hook state update
255254
expect(states[5]).toMatchObject({
256-
data: { pages: ['0-desc', '1-desc'] },
257-
isFetching: true,
258-
isFetchingNextPage: false,
259-
isSuccess: true,
260-
isPlaceholderData: true,
261-
})
262-
expect(states[6]).toMatchObject({
263255
data: { pages: ['0-asc'] },
264256
isFetching: false,
265257
isFetchingNextPage: false,

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

Lines changed: 132 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -693,17 +693,15 @@ describe('useQuery', () => {
693693
// required to make sure no additional renders are happening after data is successfully fetched for the second time
694694
await sleep(100)
695695

696-
expect(states.length).toBe(5)
696+
expect(states.length).toBe(4)
697697
// First load
698698
expect(states[0]).toMatchObject({ isPending: true, isSuccess: false })
699699
// First success
700700
expect(states[1]).toMatchObject({ isPending: false, isSuccess: true })
701701
// Remove
702702
expect(states[2]).toMatchObject({ isPending: true, isSuccess: false })
703-
// Hook state update
704-
expect(states[3]).toMatchObject({ isPending: true, isSuccess: false })
705703
// Second success
706-
expect(states[4]).toMatchObject({ isPending: false, isSuccess: true })
704+
expect(states[3]).toMatchObject({ isPending: false, isSuccess: true })
707705
})
708706

709707
it('should fetch when refetchOnMount is false and nothing has been fetched yet', async () => {
@@ -1716,7 +1714,7 @@ describe('useQuery', () => {
17161714
act(() => rendered.rerender(<Page count={2} />))
17171715
await waitFor(() => rendered.getByText('error: Error test'))
17181716

1719-
await waitFor(() => expect(states.length).toBe(8))
1717+
await waitFor(() => expect(states.length).toBe(6))
17201718
// Initial
17211719
expect(states[0]).toMatchObject({
17221720
data: undefined,
@@ -1741,46 +1739,30 @@ describe('useQuery', () => {
17411739
error: null,
17421740
isPlaceholderData: true,
17431741
})
1744-
// Hook state update
1745-
expect(states[3]).toMatchObject({
1746-
data: 0,
1747-
isFetching: true,
1748-
status: 'success',
1749-
error: null,
1750-
isPlaceholderData: true,
1751-
})
17521742
// New data
1753-
expect(states[4]).toMatchObject({
1743+
expect(states[3]).toMatchObject({
17541744
data: 1,
17551745
isFetching: false,
17561746
status: 'success',
17571747
error: null,
17581748
isPlaceholderData: false,
17591749
})
17601750
// rerender Page 2
1761-
expect(states[5]).toMatchObject({
1762-
data: 1,
1763-
isFetching: true,
1764-
status: 'success',
1765-
error: null,
1766-
isPlaceholderData: true,
1767-
})
1768-
// Hook state update again
1769-
expect(states[6]).toMatchObject({
1751+
expect(states[4]).toMatchObject({
17701752
data: 1,
17711753
isFetching: true,
17721754
status: 'success',
17731755
error: null,
17741756
isPlaceholderData: true,
17751757
})
17761758
// Error
1777-
expect(states[7]).toMatchObject({
1759+
expect(states[5]).toMatchObject({
17781760
data: undefined,
17791761
isFetching: false,
17801762
status: 'error',
17811763
isPlaceholderData: false,
17821764
})
1783-
expect(states[7]?.error).toHaveProperty('message', 'Error test')
1765+
expect(states[5]!.error).toHaveProperty('message', 'Error test')
17841766
})
17851767

17861768
it('should not show initial data from next query if placeholderData is set', async () => {
@@ -1825,7 +1807,7 @@ describe('useQuery', () => {
18251807
rendered.getByText('data: 1, count: 1, isFetching: false'),
18261808
)
18271809

1828-
expect(states.length).toBe(5)
1810+
expect(states.length).toBe(4)
18291811

18301812
// Initial
18311813
expect(states[0]).toMatchObject({
@@ -1848,15 +1830,8 @@ describe('useQuery', () => {
18481830
isSuccess: true,
18491831
isPlaceholderData: false,
18501832
})
1851-
// Hook state update
1852-
expect(states[3]).toMatchObject({
1853-
data: 99,
1854-
isFetching: true,
1855-
isSuccess: true,
1856-
isPlaceholderData: false,
1857-
})
18581833
// New data
1859-
expect(states[4]).toMatchObject({
1834+
expect(states[3]).toMatchObject({
18601835
data: 1,
18611836
isFetching: false,
18621837
isSuccess: true,
@@ -5993,4 +5968,127 @@ describe('useQuery', () => {
59935968
await waitFor(() => rendered.getByText('status: success'))
59945969
await waitFor(() => rendered.getByText('data: 1'))
59955970
})
5971+
it('should reuse same data object reference when queryKey changes back to some cached data', async () => {
5972+
const key = queryKey()
5973+
const spy = vi.fn()
5974+
5975+
async function fetchNumber(id: number) {
5976+
await sleep(5)
5977+
return { numbers: { current: { id } } }
5978+
}
5979+
function Test() {
5980+
const [id, setId] = React.useState(1)
5981+
5982+
const { data } = useQuery({
5983+
select: selector,
5984+
queryKey: [key, 'user', id],
5985+
queryFn: () => fetchNumber(id),
5986+
})
5987+
5988+
React.useEffect(() => {
5989+
spy(data)
5990+
}, [data])
5991+
5992+
return (
5993+
<div>
5994+
<button name="1" onClick={() => setId(1)}>
5995+
1
5996+
</button>
5997+
<button name="2" onClick={() => setId(2)}>
5998+
2
5999+
</button>
6000+
<span>Rendered Id: {data?.id}</span>
6001+
</div>
6002+
)
6003+
}
6004+
6005+
function selector(data: any) {
6006+
return data.numbers.current
6007+
}
6008+
6009+
const rendered = renderWithClient(queryClient, <Test />)
6010+
expect(spy).toHaveBeenCalledTimes(1)
6011+
6012+
spy.mockClear()
6013+
await waitFor(() => rendered.getByText('Rendered Id: 1'))
6014+
expect(spy).toHaveBeenCalledTimes(1)
6015+
6016+
spy.mockClear()
6017+
fireEvent.click(rendered.getByRole('button', { name: /2/ }))
6018+
await waitFor(() => rendered.getByText('Rendered Id: 2'))
6019+
expect(spy).toHaveBeenCalledTimes(2) // called with undefined because id changed
6020+
6021+
spy.mockClear()
6022+
fireEvent.click(rendered.getByRole('button', { name: /1/ }))
6023+
await waitFor(() => rendered.getByText('Rendered Id: 1'))
6024+
expect(spy).toHaveBeenCalledTimes(1)
6025+
6026+
spy.mockClear()
6027+
fireEvent.click(rendered.getByRole('button', { name: /2/ }))
6028+
await waitFor(() => rendered.getByText('Rendered Id: 2'))
6029+
expect(spy).toHaveBeenCalledTimes(1)
6030+
})
6031+
it('should reuse same data object reference when queryKey changes and placeholderData is present', async () => {
6032+
const key = queryKey()
6033+
const spy = vi.fn()
6034+
6035+
async function fetchNumber(id: number) {
6036+
await sleep(5)
6037+
return { numbers: { current: { id } } }
6038+
}
6039+
function Test() {
6040+
const [id, setId] = React.useState(1)
6041+
6042+
const { data } = useQuery({
6043+
select: selector,
6044+
queryKey: [key, 'user', id],
6045+
queryFn: () => fetchNumber(id),
6046+
placeholderData: { numbers: { current: { id: 99 } } },
6047+
})
6048+
6049+
React.useEffect(() => {
6050+
spy(data)
6051+
}, [data])
6052+
6053+
return (
6054+
<div>
6055+
<button name="1" onClick={() => setId(1)}>
6056+
1
6057+
</button>
6058+
<button name="2" onClick={() => setId(2)}>
6059+
2
6060+
</button>
6061+
<span>Rendered Id: {data?.id}</span>
6062+
</div>
6063+
)
6064+
}
6065+
6066+
function selector(data: any) {
6067+
return data.numbers.current
6068+
}
6069+
6070+
const rendered = renderWithClient(queryClient, <Test />)
6071+
expect(spy).toHaveBeenCalledTimes(1)
6072+
6073+
spy.mockClear()
6074+
await waitFor(() => rendered.getByText('Rendered Id: 99'))
6075+
await waitFor(() => rendered.getByText('Rendered Id: 1'))
6076+
expect(spy).toHaveBeenCalledTimes(1)
6077+
6078+
spy.mockClear()
6079+
fireEvent.click(rendered.getByRole('button', { name: /2/ }))
6080+
await waitFor(() => rendered.getByText('Rendered Id: 99'))
6081+
await waitFor(() => rendered.getByText('Rendered Id: 2'))
6082+
expect(spy).toHaveBeenCalledTimes(2) // called with undefined because id changed
6083+
6084+
spy.mockClear()
6085+
fireEvent.click(rendered.getByRole('button', { name: /1/ }))
6086+
await waitFor(() => rendered.getByText('Rendered Id: 1'))
6087+
expect(spy).toHaveBeenCalledTimes(1)
6088+
6089+
spy.mockClear()
6090+
fireEvent.click(rendered.getByRole('button', { name: /2/ }))
6091+
await waitFor(() => rendered.getByText('Rendered Id: 2'))
6092+
expect(spy).toHaveBeenCalledTimes(1)
6093+
})
59966094
})

‎packages/vue-query/src/__tests__/vueQueryPlugin.test.ts‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -313,8 +313,11 @@ describe('VueQueryPlugin', () => {
313313
vi.fn(),
314314
new Promise((resolve) => {
315315
setTimeout(() => {
316-
client.setQueryData(['persist'], () => ({
317-
foo: 'bar',
316+
client.setQueryData(['persist1'], () => ({
317+
foo1: 'bar1',
318+
}))
319+
client.setQueryData(['persist2'], () => ({
320+
foo2: 'bar2',
318321
}))
319322
resolve()
320323
}, 0)
@@ -324,11 +327,19 @@ describe('VueQueryPlugin', () => {
324327

325328
const fnSpy = vi.fn()
326329

330+
const query = useQuery(
331+
{
332+
queryKey: ['persist1'],
333+
queryFn: fnSpy,
334+
},
335+
customClient,
336+
)
337+
327338
const queries = useQueries(
328339
{
329340
queries: [
330341
{
331-
queryKey: ['persist'],
342+
queryKey: ['persist2'],
332343
queryFn: fnSpy,
333344
},
334345
],
@@ -337,14 +348,19 @@ describe('VueQueryPlugin', () => {
337348
)
338349

339350
expect(customClient.isRestoring.value).toBeTruthy()
351+
352+
expect(query.isFetching.value).toBeFalsy()
353+
expect(query.data.value).toStrictEqual(undefined)
354+
340355
expect(queries.value[0].isFetching).toBeFalsy()
341356
expect(queries.value[0].data).toStrictEqual(undefined)
342357
expect(fnSpy).toHaveBeenCalledTimes(0)
343358

344359
await flushPromises()
345360

346361
expect(customClient.isRestoring.value).toBeFalsy()
347-
expect(queries.value[0].data).toStrictEqual({ foo: 'bar' })
362+
expect(query.data.value).toStrictEqual({ foo1: 'bar1' })
363+
expect(queries.value[0].data).toStrictEqual({ foo2: 'bar2' })
348364
expect(fnSpy).toHaveBeenCalledTimes(0)
349365
})
350366
})

0 commit comments

Comments
 (0)