Skip to content

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

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.

Sign up for GitHub

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

Closed
TkDodo wants to merge 1 commit into from
+9 −201
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 1 addition & 72 deletions packages/query-core/src/queryObserver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,30 +240,7 @@ export class QueryObserver<
): QueryObserverResult<TData, TError> {
const query = this.client.getQueryCache().build(this.client, 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
return this.createResult(query, options)
}

getCurrentResult(): QueryObserverResult<TData, TError> {
Expand Down Expand Up @@ -793,51 +770,3 @@ 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
}
5 changes: 4 additions & 1 deletion packages/react-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx
View file
Open in desktop
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(2)
expect(states).toHaveLength(3)

expect(fetched).toBe(false)

Expand All @@ -340,6 +340,9 @@ 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
132 changes: 4 additions & 128 deletions packages/react-query/src/__tests__/useQuery.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -914,15 +914,17 @@ 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(4)
expect(states.length).toBe(5)
// 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[3]).toMatchObject({ isLoading: false, isSuccess: true })
expect(states[4]).toMatchObject({ isLoading: false, isSuccess: true })
})

it('should fetch when refetchOnMount is false and nothing has been fetched yet', async () => {
Expand Down Expand Up @@ -3815,7 +3817,6 @@ describe('useQuery', () => {
)
act(() => setPrefetched(true))
}

prefetch()
}, [])

Expand Down Expand Up @@ -6045,7 +6046,6 @@ 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 @@ -6083,132 +6083,8 @@ 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)
})
})