Skip to content

Commit

Permalink
[Fix-5538]: Assign observer's current Result when an optimistic readi…
Browse files Browse the repository at this point in the history
…ng occurs
  • Loading branch information
incepter committed Jun 13, 2023
1 parent 18c9f77 commit f1187b9
Show file tree
Hide file tree
Showing 3 changed files with 106 additions and 8 deletions.
23 changes: 22 additions & 1 deletion packages/query-core/src/queryObserver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,28 @@ 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 (!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<TData, TError> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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 () => {
Expand Down
86 changes: 83 additions & 3 deletions packages/react-query/src/__tests__/useQuery.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -914,17 +914,19 @@ 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 })
// 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 () => {
Expand Down Expand Up @@ -3650,6 +3652,7 @@ describe('useQuery', () => {
)
act(() => setPrefetched(true))
}

prefetch()
}, [])

Expand Down Expand Up @@ -5879,6 +5882,7 @@ describe('useQuery', () => {
</div>
)
}

const rendered = renderWithClient(queryClient, <Page />)
const fetchBtn = rendered.getByRole('button', { name: 'refetch' })
await waitFor(() => rendered.getByText('data: 1'))
Expand Down Expand Up @@ -5916,8 +5920,84 @@ 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 () => {
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 (
<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()
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()
})
})

0 comments on commit f1187b9

Please sign in to comment.