[Fix-5538]: Assign observer's current Result when optimistic read occurs

packages/query-core/src/queryObserver.ts

@@ -240,7 +240,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, 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<TData, TError> {
@@ -764,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<TQueryFnData, TError, TData, TQueryData, TQueryKey>,
+ optimisticResult: QueryObserverResult<TData, TError>,
+ 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
+ }
+
+ // 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
+}

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

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

@@ -914,17 +914,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({ isLoading: true, isSuccess: false })
  // First success
  expect(states[1]).toMatchObject({ isLoading: false, isSuccess: true })
  // Remove
  expect(states[2]).toMatchObject({ isLoading: true, isSuccess: false })
- // Hook state update
- 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 +3648,7 @@ describe('useQuery', () => {
  )
  act(() => setPrefetched(true))
  }
+
  prefetch()
  }, [])
 
@@ -5879,6 +5878,7 @@ describe('useQuery', () => {
  </div>
  )
  }
+
  const rendered = renderWithClient(queryClient, <Page />)
  const fetchBtn = rendered.getByRole('button', { name: 'refetch' })
  await waitFor(() => rendered.getByText('data: 1'))
@@ -5916,8 +5916,132 @@ describe('useQuery', () => {
  </div>
  )
  }
+
  const rendered = renderWithClient(queryClient, <Page />)
  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 = jest.fn()
+ const key = queryKey()
+
+ 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 spy = jest.fn()
+ const key = queryKey()
+
+ 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)
+ })
 })