From ebc1afe4345092181e9270df4e4f0a3d909c58ca Mon Sep 17 00:00:00 2001 From: TkDodo Date: Fri, 20 Jun 2025 20:24:03 +0200 Subject: [PATCH 1/6] fix: we don't want errors that get reverted internally to throw on imperative methods to achieve that, we let the retryer's `onError` "transform" the result back to potentially TData, and if so, we'll resolve the promise to that instead; that means calls to `fetchQuery` will be "successful" when a cancellation happens in the meantime, but they will then resolve to the reverted data; also, if the initial fetch is cancelled, we would still throw, as there is no data to revert to. --- .../query-core/src/__tests__/query.test.tsx | 39 +++++++++++++++++++ packages/query-core/src/query.ts | 9 ++++- packages/query-core/src/retryer.ts | 8 ++-- 3 files changed, 52 insertions(+), 4 deletions(-) diff --git a/packages/query-core/src/__tests__/query.test.tsx b/packages/query-core/src/__tests__/query.test.tsx index 25ee912637..d32a42758e 100644 --- a/packages/query-core/src/__tests__/query.test.tsx +++ b/packages/query-core/src/__tests__/query.test.tsx @@ -198,6 +198,45 @@ describe('query', () => { } }) + test('should not throw a CancelledError when fetchQuery is in progress and the last observer unsubscribes when AbortSignal is consumed', async () => { + const key = queryKey() + + const observer = new QueryObserver(queryClient, { + queryKey: key, + queryFn: async () => { + await sleep(100) + return 'data' + }, + }) + + const unsubscribe = observer.subscribe(() => undefined) + await vi.advanceTimersByTimeAsync(100) + + expect(queryCache.find({ queryKey: key })?.state.data).toBe('data') + + const promise = queryClient.fetchQuery({ + queryKey: key, + queryFn: async ({ signal }) => { + await sleep(100) + return 'data2' + String(signal) + }, + }) + + // Ensure the fetch is in progress + await vi.advanceTimersByTimeAsync(10) + + // Unsubscribe while fetch is in progress + unsubscribe() + // await queryClient.cancelQueries() + + await vi.advanceTimersByTimeAsync(90) + + // Fetch should complete successfully without throwing a CancelledError + await expect(promise).resolves.toBe('data') + + expect(queryCache.find({ queryKey: key })?.state.data).toBe('data') + }) + test('should provide context to queryFn', () => { const key = queryKey() diff --git a/packages/query-core/src/query.ts b/packages/query-core/src/query.ts index 8d6de1419d..09a9ef5237 100644 --- a/packages/query-core/src/query.ts +++ b/packages/query-core/src/query.ts @@ -495,7 +495,10 @@ export class Query< this.#dispatch({ type: 'fetch', meta: context.fetchOptions?.meta }) } - const onError = (error: TError | { silent?: boolean }) => { + const onError = ( + error: TError | { silent?: boolean }, + ): TData | undefined => { + let result: TData | undefined // Optimistically update state if needed if (!(isCancelledError(error) && error.silent)) { this.#dispatch({ @@ -515,10 +518,14 @@ export class Query< error as any, this as Query, ) + } else if (error.revert) { + // ensure error gets transformed to TData if possible after revert + result = this.state.data } // Schedule query gc after fetching this.scheduleGc() + return result } // Try to fetch the data diff --git a/packages/query-core/src/retryer.ts b/packages/query-core/src/retryer.ts index baa93aa5b4..fec4abef52 100644 --- a/packages/query-core/src/retryer.ts +++ b/packages/query-core/src/retryer.ts @@ -10,7 +10,7 @@ interface RetryerConfig { fn: () => TData | Promise initialPromise?: Promise abort?: () => void - onError?: (error: TError) => void + onError?: (error: TError) => TData | undefined onSuccess?: (data: TData) => void onFail?: (failureCount: number, error: TError) => void onPause?: () => void @@ -113,9 +113,11 @@ export function createRetryer( const reject = (value: any) => { if (!isResolved) { isResolved = true - config.onError?.(value) + const maybeResolved = config.onError?.(value) continueFn?.() - thenable.reject(value) + maybeResolved === undefined + ? thenable.reject(value) + : thenable.resolve(maybeResolved) } } From eccc9ca7e837da6a853277e56150fad2c59844cc Mon Sep 17 00:00:00 2001 From: TkDodo Date: Fri, 20 Jun 2025 23:17:02 +0200 Subject: [PATCH 2/6] refactor: async/await --- packages/query-core/src/query.ts | 132 +++++++++++++---------------- packages/query-core/src/retryer.ts | 8 +- 2 files changed, 61 insertions(+), 79 deletions(-) diff --git a/packages/query-core/src/query.ts b/packages/query-core/src/query.ts index 09a9ef5237..08706ae76c 100644 --- a/packages/query-core/src/query.ts +++ b/packages/query-core/src/query.ts @@ -168,7 +168,6 @@ export class Query< state: QueryState #initialState: QueryState - #revertState?: QueryState #cache: QueryCache #client: QueryClient #retryer?: Retryer @@ -372,7 +371,7 @@ export class Query< } } - fetch( + async fetch( options?: QueryOptions, fetchOptions?: FetchOptions, ): Promise { @@ -485,7 +484,7 @@ export class Query< this.options.behavior?.onFetch(context, this as unknown as Query) // Store state in case the current fetch needs to be reverted - this.#revertState = this.state + const revertState = this.state // Set to fetching state if not already in it if ( @@ -495,39 +494,6 @@ export class Query< this.#dispatch({ type: 'fetch', meta: context.fetchOptions?.meta }) } - const onError = ( - error: TError | { silent?: boolean }, - ): TData | undefined => { - let result: TData | undefined - // Optimistically update state if needed - if (!(isCancelledError(error) && error.silent)) { - this.#dispatch({ - type: 'error', - error: error as TError, - }) - } - - if (!isCancelledError(error)) { - // Notify cache callback - this.#cache.config.onError?.( - error as any, - this as Query, - ) - this.#cache.config.onSettled?.( - this.state.data, - error as any, - this as Query, - ) - } else if (error.revert) { - // ensure error gets transformed to TData if possible after revert - result = this.state.data - } - - // Schedule query gc after fetching - this.scheduleGc() - return result - } - // Try to fetch the data this.#retryer = createRetryer({ initialPromise: fetchOptions?.initialPromise as @@ -535,36 +501,6 @@ export class Query< | undefined, fn: context.fetchFn as () => Promise, abort: abortController.abort.bind(abortController), - onSuccess: (data) => { - if (data === undefined) { - if (process.env.NODE_ENV !== 'production') { - console.error( - `Query data cannot be undefined. Please make sure to return a value other than undefined from your query function. Affected query key: ${this.queryHash}`, - ) - } - onError(new Error(`${this.queryHash} data is undefined`) as any) - return - } - - try { - this.setData(data) - } catch (error) { - onError(error as TError) - return - } - - // Notify cache callback - this.#cache.config.onSuccess?.(data, this as Query) - this.#cache.config.onSettled?.( - data, - this.state.error as any, - this as Query, - ) - - // Schedule query gc after fetching - this.scheduleGc() - }, - onError, onFail: (failureCount, error) => { this.#dispatch({ type: 'failed', failureCount, error }) }, @@ -580,7 +516,64 @@ export class Query< canRun: () => true, }) - return this.#retryer.start() + try { + const data = await this.#retryer.start() + // this is more of a runtime guard + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + if (data === undefined) { + if (process.env.NODE_ENV !== 'production') { + console.error( + `Query data cannot be undefined. Please make sure to return a value other than undefined from your query function. Affected query key: ${this.queryHash}`, + ) + } + throw new Error(`${this.queryHash} data is undefined`) + } + + this.setData(data) + + // Notify cache callback + this.#cache.config.onSuccess?.(data, this as Query) + this.#cache.config.onSettled?.( + data, + this.state.error as any, + this as Query, + ) + return data + } catch (error) { + if (isCancelledError(error) && error.revert) { + this.setState({ + ...revertState, + fetchStatus: 'idle' as const, + }) + // transform error into reverted state data + return this.state.data! + } else { + // Optimistically update state if needed + if (!(isCancelledError(error) && error.silent)) { + this.#dispatch({ + type: 'error', + error: error as TError, + }) + } + + if (!isCancelledError(error)) { + // Notify cache callback + this.#cache.config.onError?.( + error as any, + this as Query, + ) + this.#cache.config.onSettled?.( + this.state.data, + error as any, + this as Query, + ) + } + } + throw error // rethrow the error for further handling + } finally { + // Schedule query gc after fetching + this.scheduleGc() + } } #dispatch(action: Action): void { @@ -627,11 +620,6 @@ export class Query< } case 'error': const error = action.error - - if (isCancelledError(error) && error.revert && this.#revertState) { - return { ...this.#revertState, fetchStatus: 'idle' } - } - return { ...state, error, diff --git a/packages/query-core/src/retryer.ts b/packages/query-core/src/retryer.ts index fec4abef52..6e345533af 100644 --- a/packages/query-core/src/retryer.ts +++ b/packages/query-core/src/retryer.ts @@ -10,8 +10,6 @@ interface RetryerConfig { fn: () => TData | Promise initialPromise?: Promise abort?: () => void - onError?: (error: TError) => TData | undefined - onSuccess?: (data: TData) => void onFail?: (failureCount: number, error: TError) => void onPause?: () => void onContinue?: () => void @@ -104,7 +102,6 @@ export function createRetryer( const resolve = (value: any) => { if (!isResolved) { isResolved = true - config.onSuccess?.(value) continueFn?.() thenable.resolve(value) } @@ -113,11 +110,8 @@ export function createRetryer( const reject = (value: any) => { if (!isResolved) { isResolved = true - const maybeResolved = config.onError?.(value) continueFn?.() - maybeResolved === undefined - ? thenable.reject(value) - : thenable.resolve(maybeResolved) + thenable.reject(value) } } From ee19f917b24bfa59564dd8606207efffd34fabed Mon Sep 17 00:00:00 2001 From: TkDodo Date: Fri, 20 Jun 2025 23:21:52 +0200 Subject: [PATCH 3/6] minimal test adjustments --- .../src/__tests__/hydration.test.tsx | 3 --- .../__tests__/infiniteQueryBehavior.test.tsx | 4 +++- .../query-core/src/__tests__/query.test.tsx | 22 +++++++++++++++++-- .../src/__tests__/queryClient.test.tsx | 2 +- .../src/__tests__/useQuery.test.tsx | 5 +++-- 5 files changed, 27 insertions(+), 9 deletions(-) diff --git a/packages/query-core/src/__tests__/hydration.test.tsx b/packages/query-core/src/__tests__/hydration.test.tsx index 5582e9da16..30a4b9a985 100644 --- a/packages/query-core/src/__tests__/hydration.test.tsx +++ b/packages/query-core/src/__tests__/hydration.test.tsx @@ -1398,8 +1398,5 @@ describe('dehydration and rehydration', () => { // Need to await the original promise or else it will get a cancellation // error and test will fail await originalPromise - - clientQueryClient.clear() - serverQueryClient.clear() }) }) diff --git a/packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx b/packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx index 523a30ca20..777466ef38 100644 --- a/packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx +++ b/packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx @@ -319,7 +319,9 @@ describe('InfiniteQueryBehavior', () => { // Cancel the query const query = observer.getCurrentQuery() - query.cancel() + await query.cancel() + + vi.advanceTimersByTime(10) expect(observerResult).toMatchObject({ isFetching: false, diff --git a/packages/query-core/src/__tests__/query.test.tsx b/packages/query-core/src/__tests__/query.test.tsx index d32a42758e..71e4b84028 100644 --- a/packages/query-core/src/__tests__/query.test.tsx +++ b/packages/query-core/src/__tests__/query.test.tsx @@ -5,6 +5,7 @@ import { sleep, } from '@tanstack/query-test-utils' import { + CancelledError, Query, QueryClient, QueryObserver, @@ -414,7 +415,18 @@ describe('query', () => { throw new Error() }) - queryClient.fetchQuery({ queryKey: key, queryFn, retry: 3, retryDelay: 10 }) + let error + + queryClient + .fetchQuery({ + queryKey: key, + queryFn, + retry: 3, + retryDelay: 10, + }) + .catch((e) => { + error = e + }) // Ensure the query is pending const query = queryCache.find({ queryKey: key })! @@ -429,6 +441,12 @@ describe('query', () => { expect(queryFn).toHaveBeenCalledTimes(1) // have been called, expect(query.state.error).toBe(null) // not have an error, and expect(query.state.fetchStatus).toBe('idle') // not be loading any longer + expect(query.state.data).toBe(undefined) // have no data + + // the call to fetchQuery must reject + // because it was reset and not reverted + // so it would resolve with undefined otherwise + expect(error).toBeInstanceOf(CancelledError) }) test('should reset to default state when created from hydration', async () => { @@ -939,7 +957,7 @@ describe('query', () => { unsubscribe() }) - test('should always revert to idle state (#5958)', async () => { + test('should always revert to idle state (#5968)', async () => { let mockedData = [1] const key = queryKey() diff --git a/packages/query-core/src/__tests__/queryClient.test.tsx b/packages/query-core/src/__tests__/queryClient.test.tsx index b6a5bfa81e..de3572ab3f 100644 --- a/packages/query-core/src/__tests__/queryClient.test.tsx +++ b/packages/query-core/src/__tests__/queryClient.test.tsx @@ -1050,7 +1050,7 @@ describe('queryClient', () => { queryKey: key1, queryFn: () => 'data', }) - queryClient.fetchQuery({ + queryClient.prefetchQuery({ queryKey: key1, queryFn: () => sleep(1000).then(() => 'data2'), }) diff --git a/packages/react-query/src/__tests__/useQuery.test.tsx b/packages/react-query/src/__tests__/useQuery.test.tsx index 893abb03b6..507f1a27e8 100644 --- a/packages/react-query/src/__tests__/useQuery.test.tsx +++ b/packages/react-query/src/__tests__/useQuery.test.tsx @@ -2992,7 +2992,7 @@ describe('useQuery', () => { }, retry: 2, - retryDelay: 100, + retryDelay: 200, }) return ( @@ -3036,7 +3036,8 @@ describe('useQuery', () => { expect(rendered.getByText('error: some error')).toBeInTheDocument(), ) - expect(count).toBe(4) + // 1 original fetch + 2 retries + expect(count).toBe(3) }) it('should restart when observers unmount and remount while waiting for a retry when query was cancelled in between (#3031)', async () => { From 5a1399fecd256396ea86774974aba7b376f549a2 Mon Sep 17 00:00:00 2001 From: TkDodo Date: Fri, 20 Jun 2025 23:34:53 +0200 Subject: [PATCH 4/6] test: fix it --- .../query-core/src/__tests__/query.test.tsx | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/packages/query-core/src/__tests__/query.test.tsx b/packages/query-core/src/__tests__/query.test.tsx index 71e4b84028..c1fec4c82c 100644 --- a/packages/query-core/src/__tests__/query.test.tsx +++ b/packages/query-core/src/__tests__/query.test.tsx @@ -991,18 +991,30 @@ describe('query', () => { const unsubscribe = observer.subscribe(() => undefined) await vi.advanceTimersByTimeAsync(50) // let it resolve + expect(observer.getCurrentResult().data).toBe('1') + expect(observer.getCurrentResult().fetchStatus).toBe('idle') + mockedData = [1, 2] // update "server" state in the background - queryClient.invalidateQueries({ queryKey: key }) - queryClient.invalidateQueries({ queryKey: key }) + void queryClient.invalidateQueries({ queryKey: key }) + await vi.advanceTimersByTimeAsync(5) + void queryClient.invalidateQueries({ queryKey: key }) + await vi.advanceTimersByTimeAsync(5) unsubscribe() // unsubscribe to simulate unmount + await vi.advanceTimersByTimeAsync(5) + + // reverted to previous data and idle fetchStatus + expect(queryCache.find({ queryKey: key })?.state).toMatchObject({ + status: 'success', + data: '1', + fetchStatus: 'idle', + }) // set up a new observer to simulate a mount of new component const newObserver = new QueryObserver(queryClient, { queryKey: key, queryFn, }) - const spy = vi.fn() newObserver.subscribe(({ data }) => spy(data)) await vi.advanceTimersByTimeAsync(60) // let it resolve From 8a560abf15402bc77b33c6f7272cd4824edfe2ef Mon Sep 17 00:00:00 2001 From: TkDodo Date: Fri, 20 Jun 2025 23:38:37 +0200 Subject: [PATCH 5/6] fix: revertState is now a local variable --- packages/query-core/src/query.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/query-core/src/query.ts b/packages/query-core/src/query.ts index b816071487..08706ae76c 100644 --- a/packages/query-core/src/query.ts +++ b/packages/query-core/src/query.ts @@ -604,8 +604,6 @@ export class Query< fetchMeta: action.meta ?? null, } case 'success': - // If fetching ends successfully, we don't need revertState as a fallback anymore. - this.#revertState = undefined return { ...state, data: action.data, From f2d2bbc3f3539cc416f5ce31147f08ada714aae9 Mon Sep 17 00:00:00 2001 From: TkDodo Date: Sat, 21 Jun 2025 09:41:05 +0200 Subject: [PATCH 6/6] fix: make sure "silent" reverts do not reject ongoing promises up until now, silent reverts have only stopped the query from going into error state, but the exposed promise was still rejected now, the promise won't reject because a silent revert indicates another refetch happening, so we'd want to get that promise's result instead this also means "silent" is an internal thing that shouldn't be used by consumers, because it can lead to never resolving promises instead --- .../query-core/src/__tests__/query.test.tsx | 45 ++++++++++++--- packages/query-core/src/index.ts | 1 - packages/query-core/src/query.ts | 55 ++++++++++--------- packages/query-core/src/retryer.ts | 4 -- 4 files changed, 66 insertions(+), 39 deletions(-) diff --git a/packages/query-core/src/__tests__/query.test.tsx b/packages/query-core/src/__tests__/query.test.tsx index c1fec4c82c..ee1bbc45fe 100644 --- a/packages/query-core/src/__tests__/query.test.tsx +++ b/packages/query-core/src/__tests__/query.test.tsx @@ -11,7 +11,6 @@ import { QueryObserver, dehydrate, hydrate, - isCancelledError, } from '..' import { hashQueryKeyByOptions } from '../utils' import { mockOnlineManagerIsOnline, setIsServer } from './utils' @@ -191,8 +190,7 @@ describe('query', () => { await promise expect.unreachable() } catch { - expect(isCancelledError(result)).toBe(true) - expect(result instanceof Error).toBe(true) + expect(result).toBeInstanceOf(CancelledError) } finally { // Reset visibilityState to original value visibilityMock.mockRestore() @@ -370,7 +368,7 @@ describe('query', () => { expect(signal.aborted).toBe(true) expect(onAbort).toHaveBeenCalledTimes(1) expect(abortListener).toHaveBeenCalledTimes(1) - expect(isCancelledError(error)).toBe(true) + expect(error).toBeInstanceOf(CancelledError) }) test('should not continue if explicitly cancelled', async () => { @@ -402,7 +400,7 @@ describe('query', () => { await vi.advanceTimersByTimeAsync(100) expect(queryFn).toHaveBeenCalledTimes(1) - expect(isCancelledError(error)).toBe(true) + expect(error).toBeInstanceOf(CancelledError) }) test('should not error if reset while pending', async () => { @@ -483,7 +481,7 @@ describe('query', () => { await vi.advanceTimersByTimeAsync(100) expect(queryFn).toHaveBeenCalledTimes(1) - expect(isCancelledError(query.state.error)).toBe(true) + expect(query.state.error).toBeInstanceOf(CancelledError) const result = query.fetch() await vi.advanceTimersByTimeAsync(50) await expect(result).resolves.toBe('data') @@ -516,7 +514,7 @@ describe('query', () => { await vi.advanceTimersByTimeAsync(10) expect(query.state.error).toBe(error) - expect(isCancelledError(query.state.error)).toBe(false) + expect(query.state.error).not.toBeInstanceOf(CancelledError) }) test('the previous query status should be kept when refetching', async () => { @@ -1021,6 +1019,39 @@ describe('query', () => { expect(spy).toHaveBeenCalledWith('1 - 2') }) + test('should not reject a promise when silently cancelled in the background', async () => { + const key = queryKey() + + let x = 0 + + queryClient.setQueryData(key, 'initial') + const queryFn = vi.fn().mockImplementation(async () => { + await sleep(100) + return 'data' + x + }) + + const promise = queryClient.fetchQuery({ + queryKey: key, + queryFn, + }) + + await vi.advanceTimersByTimeAsync(10) + + expect(queryFn).toHaveBeenCalledTimes(1) + + x = 1 + + // cancel ongoing re-fetches + void queryClient.refetchQueries({ queryKey: key }, { cancelRefetch: true }) + + await vi.advanceTimersByTimeAsync(10) + + // The promise should not reject + await vi.waitFor(() => expect(promise).resolves.toBe('data1')) + + expect(queryFn).toHaveBeenCalledTimes(2) + }) + it('should have an error log when queryFn data is not serializable', async () => { const consoleMock = vi.spyOn(console, 'error') diff --git a/packages/query-core/src/index.ts b/packages/query-core/src/index.ts index 889c8b02b7..f50cfb1a7a 100644 --- a/packages/query-core/src/index.ts +++ b/packages/query-core/src/index.ts @@ -26,7 +26,6 @@ export { shouldThrowError, } from './utils' export type { MutationFilters, QueryFilters, Updater, SkipToken } from './utils' -export { isCancelledError } from './retryer' export { dehydrate, hydrate, diff --git a/packages/query-core/src/query.ts b/packages/query-core/src/query.ts index 08706ae76c..623713a106 100644 --- a/packages/query-core/src/query.ts +++ b/packages/query-core/src/query.ts @@ -8,7 +8,7 @@ import { timeUntilStale, } from './utils' import { notifyManager } from './notifyManager' -import { canFetch, createRetryer, isCancelledError } from './retryer' +import { CancelledError, canFetch, createRetryer } from './retryer' import { Removable } from './removable' import type { QueryCache } from './queryCache' import type { QueryClient } from './queryClient' @@ -540,35 +540,36 @@ export class Query< ) return data } catch (error) { - if (isCancelledError(error) && error.revert) { - this.setState({ - ...revertState, - fetchStatus: 'idle' as const, - }) - // transform error into reverted state data - return this.state.data! - } else { - // Optimistically update state if needed - if (!(isCancelledError(error) && error.silent)) { - this.#dispatch({ - type: 'error', - error: error as TError, + if (error instanceof CancelledError) { + if (error.silent) { + // silent cancellation implies a new fetch is going to be started, + // so we hatch onto that promise + return this.#retryer.promise + } else if (error.revert) { + this.setState({ + ...revertState, + fetchStatus: 'idle' as const, }) - } - - if (!isCancelledError(error)) { - // Notify cache callback - this.#cache.config.onError?.( - error as any, - this as Query, - ) - this.#cache.config.onSettled?.( - this.state.data, - error as any, - this as Query, - ) + // transform error into reverted state data + return this.state.data! } } + this.#dispatch({ + type: 'error', + error: error as TError, + }) + + // Notify cache callback + this.#cache.config.onError?.( + error as any, + this as Query, + ) + this.#cache.config.onSettled?.( + this.state.data, + error as any, + this as Query, + ) + throw error // rethrow the error for further handling } finally { // Schedule query gc after fetching diff --git a/packages/query-core/src/retryer.ts b/packages/query-core/src/retryer.ts index 6e345533af..54a1dc3311 100644 --- a/packages/query-core/src/retryer.ts +++ b/packages/query-core/src/retryer.ts @@ -63,10 +63,6 @@ export class CancelledError extends Error { } } -export function isCancelledError(value: any): value is CancelledError { - return value instanceof CancelledError -} - export function createRetryer( config: RetryerConfig, ): Retryer {