Skip to content
Closed
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
87 changes: 87 additions & 0 deletions packages/query-core/src/__tests__/query.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1192,4 +1192,91 @@ 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()

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'
})

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('fetching'))

expect(query.state.fetchStatus).toBe('fetching')
})

test('should throw CancelledError when revert happens with no data after observer removal', async () => {
const key = queryKey()

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'
})

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(5)

query.removeObserver(observer1)

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

query.addObserver(observer2)
query.fetch()

await expect(promise1).rejects.toThrow(CancelledError)

expect(query.state.fetchStatus).toBe('fetching')

await vi.advanceTimersByTimeAsync(50)
expect(query.state.data).toBe('data')
})
})
17 changes: 13 additions & 4 deletions packages/query-core/src/query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,7 @@ export class Query<
// we'll let the query continue so the result can be cached
if (this.#retryer) {
if (this.#abortSignalConsumed) {
this.#retryer.cancel({ revert: true })
this.#retryer.cancel({ revert: true, isObserverRemoval: true })
} else {
this.#retryer.cancelRetry()
}
Expand Down Expand Up @@ -553,16 +553,25 @@ export class Query<
// so we hatch onto that promise
return this.#retryer.promise
} else if (error.revert) {
// If cancellation was caused by observer removal and there are active observers again,
// do not revert to idle: a new fetch may already be in flight, and reverting would
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don’t think this is a good enough check and can easily introduce regressions again. For example, you could mount an observer that won’t trigger a fetch, like a disabled one or one that has refetchOnMount: false.

Copy link
Contributor Author

@joseph0926 joseph0926 Aug 30, 2025

Choose a reason for hiding this comment

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

Although it's already closed, I've made the fix based on your feedback.

Since isActive() checks “Is there an active observer?”, while observers.length > 0 checks “Does an observer exist?”, this if-check is indeed correct.

          if (error.isObserverRemoval && this.observers.length > 0) {
            if (this.state.data === undefined) {
              throw error
            }
            return this.state.data
          }

// query.test.tsx

  test('should prevent revert when disabled observer is added after removal', async () => {
    const key = queryKey()

    const queryFn = vi.fn(async ({ signal: _signal }) => {
      await sleep(50)
      return 'data'
    })

    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,
      enabled: false,
    })

    query.addObserver(observer2)

    await expect(promise1).rejects.toBeInstanceOf(CancelledError)

    expect(query.state.fetchStatus).toBe('fetching')
    expect(query.state.data).toBeUndefined()
  })

// incorrectly flip isLoading/isFetching to false under StrictMode remounts.
if (error.isObserverRemoval && this.isActive()) {
if (this.state.data === undefined) {
throw error
}
return this.state.data
}

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

if (this.state.data === undefined) {
throw error
}

return this.state.data
}
}
Expand Down
2 changes: 2 additions & 0 deletions packages/query-core/src/retryer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,12 @@ export function canFetch(networkMode: NetworkMode | undefined): boolean {
export class CancelledError extends Error {
revert?: boolean
silent?: boolean
isObserverRemoval?: boolean
constructor(options?: CancelOptions) {
super('CancelledError')
this.revert = options?.revert
this.silent = options?.silent
this.isObserverRemoval = options?.isObserverRemoval
}
}

Expand Down
1 change: 1 addition & 0 deletions packages/query-core/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1329,6 +1329,7 @@ export interface DefaultOptions<TError = DefaultError> {
export interface CancelOptions {
revert?: boolean
silent?: boolean
isObserverRemoval?: boolean
}

export interface SetDataOptions {
Expand Down
Loading