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]>

Author: 3 people

packages/query-core/src/queryObserver.ts

@@ -227,7 +227,30 @@ export class QueryObserver<
   ): QueryObserverResult<TData, TError> {
     const query = this.#client.getQueryCache().build(this.#client, options)
 
-    return this.createResult(query, options)
+    const result = this.createResult(query, 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.
+      // 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<TData, TError> {
@@ -719,3 +742,25 @@ 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<TQueryFnData, TError, TData, TQueryData, TQueryKey>,
+  optimisticResult: QueryObserverResult<TData, TError>,
+) {
+  // 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
+}

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 () => {

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,

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(<Page count={2} />))
     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,46 +1739,30 @@ 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',
       error: null,
       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',
       error: null,
       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,127 @@ 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 key = queryKey()
+    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: [key, 'user', id],
+        queryFn: () => fetchNumber(id),
+      })
+
+      React.useEffect(() => {
+        spy(data)
+      }, [data])
+
+      return (
+        <div>
+          <button name="1" onClick={() => setId(1)}>
+            1
+          </button>
+          <button name="2" onClick={() => setId(2)}>
+            2
+          </button>
+          <span>Rendered Id: {data?.id}</span>
+        </div>
+      )
+    }
+
+    function selector(data: any) {
+      return data.numbers.current
+    }
+
+    const rendered = renderWithClient(queryClient, <Test />)
+    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)
+  })
+  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) {
+      await sleep(5)
+      return { numbers: { current: { id } } }
+    }
+    function Test() {
+      const [id, setId] = React.useState(1)
+
+      const { data } = useQuery({
+        select: selector,
+        queryKey: [key, 'user', id],
+        queryFn: () => fetchNumber(id),
+        placeholderData: { numbers: { current: { id: 99 } } },
+      })
+
+      React.useEffect(() => {
+        spy(data)
+      }, [data])
+
+      return (
+        <div>
+          <button name="1" onClick={() => setId(1)}>
+            1
+          </button>
+          <button name="2" onClick={() => setId(2)}>
+            2
+          </button>
+          <span>Rendered Id: {data?.id}</span>
+        </div>
+      )
+    }
+
+    function selector(data: any) {
+      return data.numbers.current
+    }
+
+    const rendered = renderWithClient(queryClient, <Test />)
+    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'))
+    expect(spy).toHaveBeenCalledTimes(1)
+  })
 })

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,14 +348,19 @@ 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)
 
       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)
     })
   })