Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[alpha][fix-5538] : sync Observer 'current' properties when optimistic reading occurs #5611

Merged
merged 12 commits into from
Jul 14, 2023
Merged
47 changes: 46 additions & 1 deletion packages/query-core/src/queryObserver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down Expand Up @@ -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
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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 () => {
Expand Down
10 changes: 1 addition & 9 deletions packages/react-query/src/__tests__/useInfiniteQuery.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
166 changes: 132 additions & 34 deletions packages/react-query/src/__tests__/useQuery.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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,
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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({
Expand All @@ -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,
Expand Down Expand Up @@ -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)
})
})
24 changes: 20 additions & 4 deletions packages/vue-query/src/__tests__/vueQueryPlugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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,
},
],
Expand All @@ -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)
})
})
Expand Down
Loading