Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

fix(core): make sure queries revert synchronously #9601

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

Merged
TkDodo merged 1 commit into main from feature/sync-revert
Aug 30, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
50 changes: 50 additions & 0 deletions packages/query-core/src/__tests__/query.test.tsx
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1192,4 +1192,54 @@ describe('query', () => {
expect(initialDataFn).toHaveBeenCalledTimes(1)
expect(query.state.data).toBe('initial data')
})

test('should not override fetching state when revert happens after new observer subscribes', async () => {
const key = queryKey()
let count = 0

const queryFn = vi.fn(async ({ signal: _signal }) => {
// Destructure `signal` to intentionally consume it so observer-removal uses revert-cancel path
await sleep(50)
return 'data' + count++
})

const query = new Query({
client: queryClient,
queryKey: key,
queryHash: hashQueryKeyByOptions(key),
options: { queryFn },
})

const observer1 = new QueryObserver(queryClient, {
queryKey: key,
queryFn,
})

query.addObserver(observer1)
const promise1 = query.fetch()

await vi.advanceTimersByTimeAsync(10)

query.removeObserver(observer1)

const observer2 = new QueryObserver(queryClient, {
queryKey: key,
queryFn,
})

query.addObserver(observer2)

query.fetch()

await expect(promise1).rejects.toBeInstanceOf(CancelledError)
await vi.waitFor(() => expect(query.state.fetchStatus).toBe('idle'))

expect(queryFn).toHaveBeenCalledTimes(2)

expect(query.state).toMatchObject({
fetchStatus: 'idle',
status: 'success',
data: 'data1',
})
})
Comment on lines +1196 to +1244
Copy link

@coderabbitai coderabbitai bot Aug 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix flakiness and expectation mismatch in new revert-sync test.

  • With fake timers active, waitFor won’t progress the 50ms sleep; advance timers instead to let the second fetch resolve.
  • Assert that after a microtask tick the state remains "fetching" (proves revert no longer overrides a new fetch).
  • The counter logic returns "data0" for the first completed call; make it pre-increment so the second fetch yields "data1" as asserted.

Apply this diff:

@@
- test('should not override fetching state when revert happens after new observer subscribes', async () => {
+ test('should not override fetching state when revert happens after new observer subscribes', async () => {
@@
- let count = 0
+ let count = 0
@@
- const queryFn = vi.fn(async ({ signal: _signal }) => {
- // Destructure `signal` to intentionally consume it so observer-removal uses revert-cancel path
- await sleep(50)
- return 'data' + count++
- })
+ const queryFn = vi.fn(async ({ signal: _signal }) => {
+ // Destructure `signal` to intentionally consume it so observer-removal uses revert-cancel path
+ await sleep(50)
+ return 'data' + ++count
+ })
@@
- query.fetch()
+ query.fetch()
+ // Let any queued catch handlers run; fetching state must remain
+ await Promise.resolve()
+ expect(query.state.fetchStatus).toBe('fetching')
@@
- await expect(promise1).rejects.toBeInstanceOf(CancelledError)
- await vi.waitFor(() => expect(query.state.fetchStatus).toBe('idle'))
+ await expect(promise1).rejects.toBeInstanceOf(CancelledError)
+ await vi.advanceTimersByTimeAsync(50)
+ expect(query.state.fetchStatus).toBe('idle')
@@
- expect(query.state).toMatchObject({
+ expect(query.state).toMatchObject({
 fetchStatus: 'idle',
 status: 'success',
- data: 'data1',
+ data: 'data1',
 })

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In packages/query-core/src/__tests__/query.test.tsx around lines 1196 to 1244,
the test is flaky because fake timers prevent the 50ms sleep from advancing, the
test doesn’t assert the intermediate fetching state after the revert/new fetch
overlap, and the counter returns the wrong data; replace the vi.waitFor that
expects the 50ms sleep to complete with vi.advanceTimersByTimeAsync(50) so the
second fetch can resolve, add a microtask tick (e.g., await Promise.resolve() or
equivalent) and assert query.state.fetchStatus === 'fetching' immediately after
to prove revert didn’t override the new fetch, and change the counter increment
to pre-increment so the second resolved call yields "data1" to match the final
expectation.

})
16 changes: 10 additions & 6 deletions packages/query-core/src/query.ts
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,15 @@ export class Query<
| Promise<TData>
| undefined,
fn: context.fetchFn as () => Promise<TData>,
abort: abortController.abort.bind(abortController),
onCancel: (error) => {
if (error instanceof CancelledError && error.revert) {
this.setState({
...this.#revertState,
fetchStatus: 'idle' as const,
})
}
abortController.abort()
},
onFail: (failureCount, error) => {
this.#dispatch({ type: 'failed', failureCount, error })
},
Expand Down Expand Up @@ -550,13 +558,9 @@ export class Query<
if (error instanceof CancelledError) {
if (error.silent) {
// silent cancellation implies a new fetch is going to be started,
// so we hatch onto that promise
// so we piggyback onto that promise
return this.#retryer.promise
} else if (error.revert) {
this.setState({
...this.#revertState,
fetchStatus: 'idle' as const,
})
// transform error into reverted state data
// if the initial fetch was cancelled, we have no data, so we have
// to get reject with a CancelledError
Expand Down
7 changes: 4 additions & 3 deletions packages/query-core/src/retryer.ts
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import type { CancelOptions, DefaultError, NetworkMode } from './types'
interface RetryerConfig<TData = unknown, TError = DefaultError> {
fn: () => TData | Promise<TData>
initialPromise?: Promise<TData>
abort?: () => void
onCancel?: (error: TError) => void
onFail?: (failureCount: number, error: TError) => void
onPause?: () => void
onContinue?: () => void
Expand Down Expand Up @@ -86,9 +86,10 @@ export function createRetryer<TData = unknown, TError = DefaultError>(

const cancel = (cancelOptions?: CancelOptions): void => {
if (!isResolved()) {
reject(new CancelledError(cancelOptions))
const error = new CancelledError(cancelOptions) as TError
reject(error)

config.abort?.()
config.onCancel?.(error)
}
}
const cancelRetry = () => {
Expand Down

AltStyle によって変換されたページ (->オリジナル) /