From 125a52a6752177f80c5125f1adf362fb779c959d Mon Sep 17 00:00:00 2001 From: Mohamed EL AYADI Date: Tue, 13 Jun 2023 15:05:33 +0100 Subject: [PATCH 1/4] [Fix-5538]: Assign observer's current Result when an optimistic reading occurs --- packages/query-core/src/queryObserver.ts | 23 ++++- .../PersistQueryClientProvider.test.tsx | 5 +- .../src/__tests__/useQuery.test.tsx | 86 ++++++++++++++++++- 3 files changed, 106 insertions(+), 8 deletions(-) diff --git a/packages/query-core/src/queryObserver.ts b/packages/query-core/src/queryObserver.ts index 123996e572..8affb7964b 100644 --- a/packages/query-core/src/queryObserver.ts +++ b/packages/query-core/src/queryObserver.ts @@ -240,7 +240,28 @@ 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 (!options.keepPreviousData) { + // 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 + } + return result } getCurrentResult(): QueryObserverResult { 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 3aa840221f..bce3257568 100644 --- a/packages/react-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx +++ b/packages/react-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx @@ -325,7 +325,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) @@ -340,9 +340,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__/useQuery.test.tsx b/packages/react-query/src/__tests__/useQuery.test.tsx index 2fb62e5b45..588942a9a2 100644 --- a/packages/react-query/src/__tests__/useQuery.test.tsx +++ b/packages/react-query/src/__tests__/useQuery.test.tsx @@ -914,7 +914,7 @@ 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({ isLoading: true, isSuccess: false }) // First success @@ -922,9 +922,11 @@ describe('useQuery', () => { // Remove expect(states[2]).toMatchObject({ isLoading: true, isSuccess: false }) // Hook state update - expect(states[3]).toMatchObject({ isLoading: true, isSuccess: false }) + // this update is now skipped ! because nothing changed + // see github issue: https://github.com/TanStack/query/issues/5538 + // expect(states[3]).toMatchObject({ isLoading: true, isSuccess: false }) // Second success - expect(states[4]).toMatchObject({ isLoading: false, isSuccess: true }) + expect(states[3]).toMatchObject({ isLoading: false, isSuccess: true }) }) it('should fetch when refetchOnMount is false and nothing has been fetched yet', async () => { @@ -3650,6 +3652,7 @@ describe('useQuery', () => { ) act(() => setPrefetched(true)) } + prefetch() }, []) @@ -5879,6 +5882,7 @@ describe('useQuery', () => { ) } + const rendered = renderWithClient(queryClient, ) const fetchBtn = rendered.getByRole('button', { name: 'refetch' }) await waitFor(() => rendered.getByText('data: 1')) @@ -5916,8 +5920,84 @@ describe('useQuery', () => { ) } + const rendered = renderWithClient(queryClient, ) 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 () => { + jest.useFakeTimers() + const spy = jest.fn() + + function fetchNumber(id: number) { + return new Promise((resolve) => { + setTimeout( + () => + resolve({ + numbers: { + current: { + id, + }, + }, + }), + 1000, + ) + }) + } + 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() + jest.advanceTimersByTime(1000) + await waitFor(() => rendered.getByText('Rendered Id: 1')) + expect(spy).toHaveBeenCalledTimes(1) + spy.mockClear() + + fireEvent.click(rendered.getByRole('button', { name: /2/ })) + jest.advanceTimersByTime(1000) + 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/ })) + jest.advanceTimersByTime(1000) + await waitFor(() => rendered.getByText('Rendered Id: 1')) + expect(spy).toHaveBeenCalledTimes(1) + spy.mockClear() + + fireEvent.click(rendered.getByRole('button', { name: /2/ })) + jest.advanceTimersByTime(1000) + await waitFor(() => rendered.getByText('Rendered Id: 2')) + expect(spy).toHaveBeenCalledTimes(1) + spy.mockClear() + }) }) From 3702e9b941a862c8c5dc78d9ab3f41d6fb19eaea Mon Sep 17 00:00:00 2001 From: Mohamed EL AYADI Date: Tue, 13 Jun 2023 18:55:16 +0100 Subject: [PATCH 2/4] move the condition outside so would make making decision about v4 and v5 easier & remove fake timers --- packages/query-core/src/queryObserver.ts | 52 ++++++++++++++++++- .../src/__tests__/useQuery.test.tsx | 34 +++--------- 2 files changed, 58 insertions(+), 28 deletions(-) diff --git a/packages/query-core/src/queryObserver.ts b/packages/query-core/src/queryObserver.ts index 8affb7964b..a08254424b 100644 --- a/packages/query-core/src/queryObserver.ts +++ b/packages/query-core/src/queryObserver.ts @@ -242,7 +242,7 @@ export class QueryObserver< const result = this.createResult(query, options) - if (!options.keepPreviousData) { + 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. @@ -260,6 +260,8 @@ export class QueryObserver< // 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 } @@ -785,3 +787,51 @@ 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 + >, +) { + // it is important to keep this condition like this for three reasons: + // 1. It will get removed in the v5 + // 2. it reads: don't update the properties if we want to keep the previous + // data. + // 3. The opposite condition (!options.keepPreviousData) would fallthrough + // and will result in a bad decision + if (options.keepPreviousData) { + return false + } + + // 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 + } + + // 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 + } + + // basically, just keep previous properties if nothing changed + return false +} diff --git a/packages/react-query/src/__tests__/useQuery.test.tsx b/packages/react-query/src/__tests__/useQuery.test.tsx index 588942a9a2..412e1b196d 100644 --- a/packages/react-query/src/__tests__/useQuery.test.tsx +++ b/packages/react-query/src/__tests__/useQuery.test.tsx @@ -921,10 +921,6 @@ describe('useQuery', () => { expect(states[1]).toMatchObject({ isLoading: false, isSuccess: true }) // Remove expect(states[2]).toMatchObject({ isLoading: true, isSuccess: false }) - // Hook state update - // this update is now skipped ! because nothing changed - // see github issue: https://github.com/TanStack/query/issues/5538 - // expect(states[3]).toMatchObject({ isLoading: true, isSuccess: false }) // Second success expect(states[3]).toMatchObject({ isLoading: false, isSuccess: true }) }) @@ -5926,23 +5922,11 @@ describe('useQuery', () => { await waitFor(() => rendered.getByText('data: 1')) }) it('should reuse same data object reference when queryKey changes back to some cached data', async () => { - jest.useFakeTimers() const spy = jest.fn() - function fetchNumber(id: number) { - return new Promise((resolve) => { - setTimeout( - () => - resolve({ - numbers: { - current: { - id, - }, - }, - }), - 1000, - ) - }) + async function fetchNumber(id: number) { + await sleep(5) + return { numbers: { current: { id } } } } function Test() { const [id, setId] = React.useState(1) @@ -5976,28 +5960,24 @@ describe('useQuery', () => { const rendered = renderWithClient(queryClient, ) expect(spy).toHaveBeenCalledTimes(1) + spy.mockClear() - jest.advanceTimersByTime(1000) await waitFor(() => rendered.getByText('Rendered Id: 1')) expect(spy).toHaveBeenCalledTimes(1) - spy.mockClear() + spy.mockClear() fireEvent.click(rendered.getByRole('button', { name: /2/ })) - jest.advanceTimersByTime(1000) await waitFor(() => rendered.getByText('Rendered Id: 2')) expect(spy).toHaveBeenCalledTimes(2) // called with undefined because id changed - spy.mockClear() + spy.mockClear() fireEvent.click(rendered.getByRole('button', { name: /1/ })) - jest.advanceTimersByTime(1000) await waitFor(() => rendered.getByText('Rendered Id: 1')) expect(spy).toHaveBeenCalledTimes(1) - spy.mockClear() + spy.mockClear() fireEvent.click(rendered.getByRole('button', { name: /2/ })) - jest.advanceTimersByTime(1000) await waitFor(() => rendered.getByText('Rendered Id: 2')) expect(spy).toHaveBeenCalledTimes(1) - spy.mockClear() }) }) From 622adc9283081bbe9c5c41e284a8bcf00fb57f1b Mon Sep 17 00:00:00 2001 From: Mohamed EL AYADI Date: Tue, 20 Jun 2023 19:17:22 +0100 Subject: [PATCH 3/4] Flip test on currentResult with placeholderData's order & add test for placeholderData --- packages/query-core/src/queryObserver.ts | 12 ++-- .../src/__tests__/useQuery.test.tsx | 62 +++++++++++++++++++ 2 files changed, 68 insertions(+), 6 deletions(-) diff --git a/packages/query-core/src/queryObserver.ts b/packages/query-core/src/queryObserver.ts index a08254424b..4e4ace8f8b 100644 --- a/packages/query-core/src/queryObserver.ts +++ b/packages/query-core/src/queryObserver.ts @@ -817,12 +817,6 @@ function shouldAssignObserverCurrentProperties< return false } - // 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 - } - // this means we want to put some placeholder data when pending and queryKey // changed. if (options.placeholderData !== undefined) { @@ -832,6 +826,12 @@ function shouldAssignObserverCurrentProperties< 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/src/__tests__/useQuery.test.tsx b/packages/react-query/src/__tests__/useQuery.test.tsx index 412e1b196d..6e4e0329ca 100644 --- a/packages/react-query/src/__tests__/useQuery.test.tsx +++ b/packages/react-query/src/__tests__/useQuery.test.tsx @@ -5975,6 +5975,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 = jest.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 2ef90213d7f93fbe6afd8a704b63fce686ae05b4 Mon Sep 17 00:00:00 2001 From: Mohamed EL AYADI Date: Fri, 23 Jun 2023 15:38:43 +0100 Subject: [PATCH 4/4] 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 6e4e0329ca..b5550d8858 100644 --- a/packages/react-query/src/__tests__/useQuery.test.tsx +++ b/packages/react-query/src/__tests__/useQuery.test.tsx @@ -5923,6 +5923,7 @@ describe('useQuery', () => { }) it('should reuse same data object reference when queryKey changes back to some cached data', async () => { const spy = jest.fn() + const key = queryKey() async function fetchNumber(id: number) { await sleep(5) @@ -5933,7 +5934,7 @@ describe('useQuery', () => { const { data } = useQuery({ select: selector, - queryKey: ['user', id], + queryKey: [key, 'user', id], queryFn: () => fetchNumber(id), }) @@ -5982,6 +5983,7 @@ describe('useQuery', () => { }) it('should reuse same data object reference when queryKey changes and placeholderData is present', async () => { const spy = jest.fn() + const key = queryKey() async function fetchNumber(id: number) { await sleep(5) @@ -5992,7 +5994,7 @@ describe('useQuery', () => { const { data } = useQuery({ select: selector, - queryKey: ['user', id], + queryKey: [key, 'user', id], queryFn: () => fetchNumber(id), placeholderData: { numbers: { current: { id: 99 } } }, })