From c9712d46eaa8d7b3d97b122e761f742e773873d6 Mon Sep 17 00:00:00 2001 From: Mohamed EL AYADI Date: Mon, 19 Jun 2023 12:47:14 +0100 Subject: [PATCH 1/8] fix-5538 : sync Observer 'current' properties when optimistic reading occurs --- packages/query-core/src/queryObserver.ts | 63 ++++++++++- .../PersistQueryClientProvider.test.tsx | 5 +- .../src/__tests__/useInfiniteQuery.test.tsx | 10 +- .../src/__tests__/useQuery.test.tsx | 102 ++++++++++++------ 4 files changed, 132 insertions(+), 48 deletions(-) diff --git a/packages/query-core/src/queryObserver.ts b/packages/query-core/src/queryObserver.ts index 9f1dbee919..0d1f2022a6 100644 --- a/packages/query-core/src/queryObserver.ts +++ b/packages/query-core/src/queryObserver.ts @@ -227,7 +227,30 @@ export class QueryObserver< ): QueryObserverResult { const query = this.#client.getQueryCache().build(this.#client, options) - return this.createResult(query, options) + const result = this.createResult(query, options) + + if (shouldAssignObserverCurrentProperties(this, result, options)) { + // this assigns the optimistic result to the current Observer + // because if the query function changes, useQuery will be performing + // an effect where it would fetch again. + // When the fetch finishes, we perform a deep data cloning in order + // to reuse objects references. This deep data clone is performed against + // the `observer.currentResult.data` property + // When QueryKey changes, we refresh the query and get new `optimistic` + // result, while we leave the `observer.currentResult`, so when new data + // arrives, it finds the old `observer.currentResult` which is related + // to the old QueryKey. Which means that currentResult and selectData are + // out of sync already. + // To solve this, we move the cursor of the currentResult everytime + // an observer reads an optimistic value. + + // When keeping the previous data, the result doesn't change until new + // data arrives. + this.#currentResult = result + this.#currentResultOptions = this.options + this.#currentResultState = this.#currentQuery.state + } + return result } getCurrentResult(): QueryObserverResult { @@ -719,3 +742,41 @@ function isStale( ): boolean { return query.isStaleByTime(options.staleTime) } + +// this function would decide if we will update the observer's 'current' +// properties after an optimistic reading via getOptimisticResult +function shouldAssignObserverCurrentProperties< + TQueryFnData = unknown, + TError = unknown, + TData = TQueryFnData, + TQueryData = TQueryFnData, + TQueryKey extends QueryKey = QueryKey, +>( + observer: QueryObserver, + optimisticResult: QueryObserverResult, + options: DefaultedQueryObserverOptions< + TQueryFnData, + TError, + TData, + TQueryData, + TQueryKey + >, +) { + // this means we want to put some placeholder data when pending and queryKey + // changed. + if (options.placeholderData !== undefined) { + // re-assign properties only if current data is placeholder data + // which means that data did not arrive yet, so, if there is some cached data + // we need to "prepare" to receive it + return optimisticResult.isPlaceholderData + } + + // if the newly created result isn't what the observer is holding as current, + // then we'll need to update the properties as well + if (observer.getCurrentResult() !== optimisticResult) { + return true + } + + // basically, just keep previous properties if nothing changed + return false +} diff --git a/packages/react-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx b/packages/react-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx index f1e617ea25..721d9bf3cb 100644 --- a/packages/react-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx +++ b/packages/react-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx @@ -339,7 +339,7 @@ describe('PersistQueryClientProvider', () => { await waitFor(() => rendered.getByText('data: null')) await waitFor(() => rendered.getByText('data: hydrated')) - expect(states).toHaveLength(3) + expect(states).toHaveLength(2) expect(fetched).toBe(false) @@ -354,9 +354,6 @@ describe('PersistQueryClientProvider', () => { fetchStatus: 'idle', data: 'hydrated', }) - - // #5443 seems like we get an extra render now ... - expect(states[1]).toStrictEqual(states[2]) }) test('should call onSuccess after successful restoring', async () => { diff --git a/packages/react-query/src/__tests__/useInfiniteQuery.test.tsx b/packages/react-query/src/__tests__/useInfiniteQuery.test.tsx index d2a0c7785a..38b8134c7c 100644 --- a/packages/react-query/src/__tests__/useInfiniteQuery.test.tsx +++ b/packages/react-query/src/__tests__/useInfiniteQuery.test.tsx @@ -213,7 +213,7 @@ describe('useInfiniteQuery', () => { await waitFor(() => rendered.getByText('data: 0-asc')) await waitFor(() => rendered.getByText('isFetching: false')) - await waitFor(() => expect(states.length).toBe(7)) + await waitFor(() => expect(states.length).toBe(6)) expect(states[0]).toMatchObject({ data: undefined, @@ -251,15 +251,7 @@ describe('useInfiniteQuery', () => { isSuccess: true, isPlaceholderData: true, }) - // Hook state update expect(states[5]).toMatchObject({ - data: { pages: ['0-desc', '1-desc'] }, - isFetching: true, - isFetchingNextPage: false, - isSuccess: true, - isPlaceholderData: true, - }) - expect(states[6]).toMatchObject({ data: { pages: ['0-asc'] }, isFetching: false, isFetchingNextPage: false, diff --git a/packages/react-query/src/__tests__/useQuery.test.tsx b/packages/react-query/src/__tests__/useQuery.test.tsx index 207bc9dd09..de993b4788 100644 --- a/packages/react-query/src/__tests__/useQuery.test.tsx +++ b/packages/react-query/src/__tests__/useQuery.test.tsx @@ -693,17 +693,15 @@ describe('useQuery', () => { // required to make sure no additional renders are happening after data is successfully fetched for the second time await sleep(100) - expect(states.length).toBe(5) + expect(states.length).toBe(4) // First load expect(states[0]).toMatchObject({ isPending: true, isSuccess: false }) // First success expect(states[1]).toMatchObject({ isPending: false, isSuccess: true }) // Remove expect(states[2]).toMatchObject({ isPending: true, isSuccess: false }) - // Hook state update - expect(states[3]).toMatchObject({ isPending: true, isSuccess: false }) // Second success - expect(states[4]).toMatchObject({ isPending: false, isSuccess: true }) + expect(states[3]).toMatchObject({ isPending: false, isSuccess: true }) }) it('should fetch when refetchOnMount is false and nothing has been fetched yet', async () => { @@ -1716,7 +1714,7 @@ describe('useQuery', () => { act(() => rendered.rerender()) await waitFor(() => rendered.getByText('error: Error test')) - await waitFor(() => expect(states.length).toBe(8)) + await waitFor(() => expect(states.length).toBe(6)) // Initial expect(states[0]).toMatchObject({ data: undefined, @@ -1741,16 +1739,8 @@ describe('useQuery', () => { error: null, isPlaceholderData: true, }) - // Hook state update - expect(states[3]).toMatchObject({ - data: 0, - isFetching: true, - status: 'success', - error: null, - isPlaceholderData: true, - }) // New data - expect(states[4]).toMatchObject({ + expect(states[3]).toMatchObject({ data: 1, isFetching: false, status: 'success', @@ -1758,15 +1748,7 @@ describe('useQuery', () => { isPlaceholderData: false, }) // rerender Page 2 - expect(states[5]).toMatchObject({ - data: 1, - isFetching: true, - status: 'success', - error: null, - isPlaceholderData: true, - }) - // Hook state update again - expect(states[6]).toMatchObject({ + expect(states[4]).toMatchObject({ data: 1, isFetching: true, status: 'success', @@ -1774,13 +1756,13 @@ describe('useQuery', () => { isPlaceholderData: true, }) // Error - expect(states[7]).toMatchObject({ + expect(states[5]).toMatchObject({ data: undefined, isFetching: false, status: 'error', isPlaceholderData: false, }) - expect(states[7]?.error).toHaveProperty('message', 'Error test') + expect(states[5]!.error).toHaveProperty('message', 'Error test') }) it('should not show initial data from next query if placeholderData is set', async () => { @@ -1825,7 +1807,7 @@ describe('useQuery', () => { rendered.getByText('data: 1, count: 1, isFetching: false'), ) - expect(states.length).toBe(5) + expect(states.length).toBe(4) // Initial expect(states[0]).toMatchObject({ @@ -1848,15 +1830,8 @@ describe('useQuery', () => { isSuccess: true, isPlaceholderData: false, }) - // Hook state update - expect(states[3]).toMatchObject({ - data: 99, - isFetching: true, - isSuccess: true, - isPlaceholderData: false, - }) // New data - expect(states[4]).toMatchObject({ + expect(states[3]).toMatchObject({ data: 1, isFetching: false, isSuccess: true, @@ -5993,4 +5968,63 @@ describe('useQuery', () => { await waitFor(() => rendered.getByText('status: success')) await waitFor(() => rendered.getByText('data: 1')) }) + it('should reuse same data object reference when queryKey changes back to some cached data', async () => { + const spy = vi.fn() + + async function fetchNumber(id: number) { + await sleep(5) + return { numbers: { current: { id } } } + } + function Test() { + const [id, setId] = React.useState(1) + + const { data } = useQuery({ + select: selector, + queryKey: ['user', id], + queryFn: () => fetchNumber(id), + }) + + React.useEffect(() => { + spy(data) + }, [data]) + + return ( +
+ + + Rendered Id: {data?.id} +
+ ) + } + + function selector(data: any) { + return data.numbers.current + } + + const rendered = renderWithClient(queryClient, ) + expect(spy).toHaveBeenCalledTimes(1) + + spy.mockClear() + await waitFor(() => rendered.getByText('Rendered Id: 1')) + expect(spy).toHaveBeenCalledTimes(1) + + spy.mockClear() + fireEvent.click(rendered.getByRole('button', { name: /2/ })) + await waitFor(() => rendered.getByText('Rendered Id: 2')) + expect(spy).toHaveBeenCalledTimes(2) // called with undefined because id changed + + spy.mockClear() + fireEvent.click(rendered.getByRole('button', { name: /1/ })) + await waitFor(() => rendered.getByText('Rendered Id: 1')) + expect(spy).toHaveBeenCalledTimes(1) + + spy.mockClear() + fireEvent.click(rendered.getByRole('button', { name: /2/ })) + await waitFor(() => rendered.getByText('Rendered Id: 2')) + expect(spy).toHaveBeenCalledTimes(1) + }) }) From ead1b5930d7b2998df3e56ed51d2e88d6b05f631 Mon Sep 17 00:00:00 2001 From: Mohamed EL AYADI Date: Tue, 20 Jun 2023 19:17:22 +0100 Subject: [PATCH 2/8] Flip test on currentResult with placeholderData's order & add test --- .../src/__tests__/useQuery.test.tsx | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/packages/react-query/src/__tests__/useQuery.test.tsx b/packages/react-query/src/__tests__/useQuery.test.tsx index de993b4788..c59e76ea01 100644 --- a/packages/react-query/src/__tests__/useQuery.test.tsx +++ b/packages/react-query/src/__tests__/useQuery.test.tsx @@ -6022,6 +6022,68 @@ describe('useQuery', () => { await waitFor(() => rendered.getByText('Rendered Id: 1')) expect(spy).toHaveBeenCalledTimes(1) + spy.mockClear() + fireEvent.click(rendered.getByRole('button', { name: /2/ })) + await waitFor(() => rendered.getByText('Rendered Id: 2')) + expect(spy).toHaveBeenCalledTimes(1) + }) + it('should reuse same data object reference when queryKey changes and placeholderData is present', async () => { + const spy = vi.fn() + + async function fetchNumber(id: number) { + await sleep(5) + return { numbers: { current: { id } } } + } + function Test() { + const [id, setId] = React.useState(1) + + const { data } = useQuery({ + select: selector, + queryKey: ['user', id], + queryFn: () => fetchNumber(id), + placeholderData: { numbers: { current: { id: 99 } } }, + }) + + React.useEffect(() => { + spy(data) + }, [data]) + + return ( +
+ + + Rendered Id: {data?.id} +
+ ) + } + + function selector(data: any) { + return data.numbers.current + } + + const rendered = renderWithClient(queryClient, ) + expect(spy).toHaveBeenCalledTimes(1) + + spy.mockClear() + await waitFor(() => rendered.getByText('Rendered Id: 99')) + await waitFor(() => rendered.getByText('Rendered Id: 1')) + expect(spy).toHaveBeenCalledTimes(1) + + spy.mockClear() + fireEvent.click(rendered.getByRole('button', { name: /2/ })) + await waitFor(() => rendered.getByText('Rendered Id: 99')) + await waitFor(() => rendered.getByText('Rendered Id: 2')) + expect(spy).toHaveBeenCalledTimes(2) // called with undefined because id changed + + spy.mockClear() + fireEvent.click(rendered.getByRole('button', { name: /1/ })) + await waitFor(() => rendered.getByText('Rendered Id: 1')) + expect(spy).toHaveBeenCalledTimes(1) + spy.mockClear() fireEvent.click(rendered.getByRole('button', { name: /2/ })) await waitFor(() => rendered.getByText('Rendered Id: 2')) From a0170168210002e7e2c06b4e921bcbf324a63d61 Mon Sep 17 00:00:00 2001 From: Mohamed EL AYADI Date: Fri, 23 Jun 2023 15:38:43 +0100 Subject: [PATCH 3/8] Fix tests using the same queryKey --- packages/react-query/src/__tests__/useQuery.test.tsx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/react-query/src/__tests__/useQuery.test.tsx b/packages/react-query/src/__tests__/useQuery.test.tsx index c59e76ea01..c31a5b8cdf 100644 --- a/packages/react-query/src/__tests__/useQuery.test.tsx +++ b/packages/react-query/src/__tests__/useQuery.test.tsx @@ -5969,6 +5969,7 @@ describe('useQuery', () => { await waitFor(() => rendered.getByText('data: 1')) }) it('should reuse same data object reference when queryKey changes back to some cached data', async () => { + const key = queryKey() const spy = vi.fn() async function fetchNumber(id: number) { @@ -5980,7 +5981,7 @@ describe('useQuery', () => { const { data } = useQuery({ select: selector, - queryKey: ['user', id], + queryKey: [key, 'user', id], queryFn: () => fetchNumber(id), }) @@ -6028,6 +6029,7 @@ describe('useQuery', () => { expect(spy).toHaveBeenCalledTimes(1) }) it('should reuse same data object reference when queryKey changes and placeholderData is present', async () => { + const key = queryKey() const spy = vi.fn() async function fetchNumber(id: number) { @@ -6039,7 +6041,7 @@ describe('useQuery', () => { const { data } = useQuery({ select: selector, - queryKey: ['user', id], + queryKey: [key, 'user', id], queryFn: () => fetchNumber(id), placeholderData: { numbers: { current: { id: 99 } } }, }) From 14703aaf015ee6c25224868844fafb02947e5a93 Mon Sep 17 00:00:00 2001 From: Mohamed EL AYADI Date: Mon, 26 Jun 2023 10:06:14 +0100 Subject: [PATCH 4/8] remove options.placeHolderData from deciding whether to assign observer current properties, use equalitycheck on v5 --- packages/query-core/src/queryObserver.ts | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/packages/query-core/src/queryObserver.ts b/packages/query-core/src/queryObserver.ts index 0d1f2022a6..58c902d580 100644 --- a/packages/query-core/src/queryObserver.ts +++ b/packages/query-core/src/queryObserver.ts @@ -229,7 +229,7 @@ export class QueryObserver< const result = this.createResult(query, options) - if (shouldAssignObserverCurrentProperties(this, result, options)) { + if (shouldAssignObserverCurrentProperties(this, result)) { // this assigns the optimistic result to the current Observer // because if the query function changes, useQuery will be performing // an effect where it would fetch again. @@ -754,23 +754,7 @@ function shouldAssignObserverCurrentProperties< >( observer: QueryObserver, optimisticResult: QueryObserverResult, - options: DefaultedQueryObserverOptions< - TQueryFnData, - TError, - TData, - TQueryData, - TQueryKey - >, ) { - // this means we want to put some placeholder data when pending and queryKey - // changed. - if (options.placeholderData !== undefined) { - // re-assign properties only if current data is placeholder data - // which means that data did not arrive yet, so, if there is some cached data - // we need to "prepare" to receive it - return optimisticResult.isPlaceholderData - } - // if the newly created result isn't what the observer is holding as current, // then we'll need to update the properties as well if (observer.getCurrentResult() !== optimisticResult) { From 3930efcf4b18d7a1341621f081dd4319768f89a8 Mon Sep 17 00:00:00 2001 From: Mohamed EL AYADI Date: Mon, 26 Jun 2023 21:29:01 +0100 Subject: [PATCH 5/8] maybe fix for vue reactivity cbs --- packages/vue-query/src/useQueries.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/vue-query/src/useQueries.ts b/packages/vue-query/src/useQueries.ts index b4f9501579..c3d3e041c1 100644 --- a/packages/vue-query/src/useQueries.ts +++ b/packages/vue-query/src/useQueries.ts @@ -215,7 +215,10 @@ export function useQueries< defaultedQueries.value, options as QueriesObserverOptions, ) - state.value = observer.getCurrentResult() + const [, getCombinedResultPersisted] = observer.getOptimisticResult( + defaultedQueries.value, + ) + state.value = getCombinedResultPersisted() }, { deep: true }, ) From 2f99fcad8220243db0c30d28242bae17eb3d2db1 Mon Sep 17 00:00:00 2001 From: Damian Osipiuk Date: Fri, 30 Jun 2023 23:38:48 +0200 Subject: [PATCH 6/8] test(vue-query): test persister with useQuery --- .../src/__tests__/vueQueryPlugin.test.ts | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/packages/vue-query/src/__tests__/vueQueryPlugin.test.ts b/packages/vue-query/src/__tests__/vueQueryPlugin.test.ts index 7ef3bed2b6..c6ebb6815f 100644 --- a/packages/vue-query/src/__tests__/vueQueryPlugin.test.ts +++ b/packages/vue-query/src/__tests__/vueQueryPlugin.test.ts @@ -313,8 +313,11 @@ describe('VueQueryPlugin', () => { vi.fn(), new Promise((resolve) => { setTimeout(() => { - client.setQueryData(['persist'], () => ({ - foo: 'bar', + client.setQueryData(['persist1'], () => ({ + foo1: 'bar1', + })) + client.setQueryData(['persist2'], () => ({ + foo2: 'bar2', })) resolve() }, 0) @@ -324,11 +327,19 @@ describe('VueQueryPlugin', () => { const fnSpy = vi.fn() + const query = useQuery( + { + queryKey: ['persist1'], + queryFn: fnSpy, + }, + customClient, + ) + const queries = useQueries( { queries: [ { - queryKey: ['persist'], + queryKey: ['persist2'], queryFn: fnSpy, }, ], @@ -337,6 +348,10 @@ describe('VueQueryPlugin', () => { ) expect(customClient.isRestoring.value).toBeTruthy() + + expect(query.isFetching.value).toBeFalsy() + expect(query.data.value).toStrictEqual(undefined) + expect(queries.value[0].isFetching).toBeFalsy() expect(queries.value[0].data).toStrictEqual(undefined) expect(fnSpy).toHaveBeenCalledTimes(0) @@ -344,7 +359,8 @@ describe('VueQueryPlugin', () => { await flushPromises() expect(customClient.isRestoring.value).toBeFalsy() - expect(queries.value[0].data).toStrictEqual({ foo: 'bar' }) + expect(query.data.value).toStrictEqual({ foo1: 'bar1' }) + expect(queries.value[0].data).toStrictEqual({ foo2: 'bar2' }) expect(fnSpy).toHaveBeenCalledTimes(0) }) }) From 6f0a469f51e162052ea3245b4ef3da7c6febc9c3 Mon Sep 17 00:00:00 2001 From: Mohamed EL AYADI Date: Mon, 3 Jul 2023 11:51:05 +0100 Subject: [PATCH 7/8] Force line terminator to \\n on codemods tests --- packages/codemods/src/v4/key-transformation.js | 2 +- packages/codemods/src/v4/replace-import-specifier.js | 2 +- packages/codemods/src/v5/remove-overloads/remove-overloads.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/codemods/src/v4/key-transformation.js b/packages/codemods/src/v4/key-transformation.js index 3ebb2dfc07..a6931043d7 100644 --- a/packages/codemods/src/v4/key-transformation.js +++ b/packages/codemods/src/v4/key-transformation.js @@ -134,5 +134,5 @@ module.exports = (file, api) => { // This function transforms usages of `QueryCache`. transformQueryCacheUsages({ jscodeshift, utils, root, filePath }) - return root.toSource({ quote: 'single' }) + return root.toSource({ quote: 'single', lineTerminator: '\n' }) } diff --git a/packages/codemods/src/v4/replace-import-specifier.js b/packages/codemods/src/v4/replace-import-specifier.js index e35ec3a44b..754e2f7bd5 100644 --- a/packages/codemods/src/v4/replace-import-specifier.js +++ b/packages/codemods/src/v4/replace-import-specifier.js @@ -21,5 +21,5 @@ module.exports = (file, api) => { }) }) - return root.toSource({ quote: 'single' }) + return root.toSource({ quote: 'single', lineTerminator: '\n' }) } diff --git a/packages/codemods/src/v5/remove-overloads/remove-overloads.js b/packages/codemods/src/v5/remove-overloads/remove-overloads.js index 34bd6dada8..2cc5bd8555 100644 --- a/packages/codemods/src/v5/remove-overloads/remove-overloads.js +++ b/packages/codemods/src/v5/remove-overloads/remove-overloads.js @@ -55,5 +55,5 @@ module.exports = (file, api) => { }, }) - return root.toSource({ quote: 'single' }) + return root.toSource({ quote: 'single', lineTerminator: '\n' }) } From a830410f6d7ce5b9809134422e582d1f9d36d9b1 Mon Sep 17 00:00:00 2001 From: Mohamed EL AYADI Date: Mon, 3 Jul 2023 11:58:24 +0100 Subject: [PATCH 8/8] Revert " Force line terminator to \\n on codemods tests" This reverts commit 6f0a469f51e162052ea3245b4ef3da7c6febc9c3. --- packages/codemods/src/v4/key-transformation.js | 2 +- packages/codemods/src/v4/replace-import-specifier.js | 2 +- packages/codemods/src/v5/remove-overloads/remove-overloads.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/codemods/src/v4/key-transformation.js b/packages/codemods/src/v4/key-transformation.js index a6931043d7..3ebb2dfc07 100644 --- a/packages/codemods/src/v4/key-transformation.js +++ b/packages/codemods/src/v4/key-transformation.js @@ -134,5 +134,5 @@ module.exports = (file, api) => { // This function transforms usages of `QueryCache`. transformQueryCacheUsages({ jscodeshift, utils, root, filePath }) - return root.toSource({ quote: 'single', lineTerminator: '\n' }) + return root.toSource({ quote: 'single' }) } diff --git a/packages/codemods/src/v4/replace-import-specifier.js b/packages/codemods/src/v4/replace-import-specifier.js index 754e2f7bd5..e35ec3a44b 100644 --- a/packages/codemods/src/v4/replace-import-specifier.js +++ b/packages/codemods/src/v4/replace-import-specifier.js @@ -21,5 +21,5 @@ module.exports = (file, api) => { }) }) - return root.toSource({ quote: 'single', lineTerminator: '\n' }) + return root.toSource({ quote: 'single' }) } diff --git a/packages/codemods/src/v5/remove-overloads/remove-overloads.js b/packages/codemods/src/v5/remove-overloads/remove-overloads.js index 2cc5bd8555..34bd6dada8 100644 --- a/packages/codemods/src/v5/remove-overloads/remove-overloads.js +++ b/packages/codemods/src/v5/remove-overloads/remove-overloads.js @@ -55,5 +55,5 @@ module.exports = (file, api) => { }, }) - return root.toSource({ quote: 'single', lineTerminator: '\n' }) + return root.toSource({ quote: 'single' }) }