From d43557cbf4189d93af6ad5e84b2f71682430aa2e Mon Sep 17 00:00:00 2001 From: Dominik Dorfmeister Date: Sat, 13 Nov 2021 13:20:24 +0100 Subject: [PATCH 1/9] feat: streamline cancelRefetch the following functions now default to true for cancelRefetch: - refetchQueries (+invalidateQueries, + resetQueries) - query.refetch - fetchNextPage (unchanged) - fetchPreviousPage (unchanged) --- src/core/infiniteQueryObserver.ts | 2 -- src/core/queryClient.ts | 1 + src/core/queryObserver.ts | 1 + 3 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/infiniteQueryObserver.ts b/src/core/infiniteQueryObserver.ts index 19ef264650..fc76ded7db 100644 --- a/src/core/infiniteQueryObserver.ts +++ b/src/core/infiniteQueryObserver.ts @@ -94,7 +94,6 @@ export class InfiniteQueryObserver< options?: FetchNextPageOptions ): Promise> { return this.fetch({ - // TODO consider removing `?? true` in future breaking change, to be consistent with `refetch` API (see https://github.com/tannerlinsley/react-query/issues/2617) cancelRefetch: options?.cancelRefetch ?? true, throwOnError: options?.throwOnError, meta: { @@ -107,7 +106,6 @@ export class InfiniteQueryObserver< options?: FetchPreviousPageOptions ): Promise> { return this.fetch({ - // TODO consider removing `?? true` in future breaking change, to be consistent with `refetch` API (see https://github.com/tannerlinsley/react-query/issues/2617) cancelRefetch: options?.cancelRefetch ?? true, throwOnError: options?.throwOnError, meta: { diff --git a/src/core/queryClient.ts b/src/core/queryClient.ts index cb9d6ba181..d6e973136a 100644 --- a/src/core/queryClient.ts +++ b/src/core/queryClient.ts @@ -291,6 +291,7 @@ export class QueryClient { this.queryCache.findAll(filters).map(query => query.fetch(undefined, { ...options, + cancelRefetch: options?.cancelRefetch ?? true, meta: { refetchPage: filters?.refetchPage }, }) ) diff --git a/src/core/queryObserver.ts b/src/core/queryObserver.ts index f1ab5c8d2b..b43c0681e1 100644 --- a/src/core/queryObserver.ts +++ b/src/core/queryObserver.ts @@ -283,6 +283,7 @@ export class QueryObserver< ): Promise> { return this.fetch({ ...options, + cancelRefetch: options?.cancelRefetch ?? true, meta: { refetchPage: options?.refetchPage }, }) } From 41c9b36630365ddadca34be7c223311f1b7f1a11 Mon Sep 17 00:00:00 2001 From: Dominik Dorfmeister Date: Sat, 13 Nov 2021 14:16:43 +0100 Subject: [PATCH 2/9] feat: streamline cancelRefetch make sure that refetchOnReconnect and refetchOnWindowFocus do not cancel already running requests --- src/core/query.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/query.ts b/src/core/query.ts index 336e9b8748..a466b137bc 100644 --- a/src/core/query.ts +++ b/src/core/query.ts @@ -297,7 +297,7 @@ export class Query< const observer = this.observers.find(x => x.shouldFetchOnWindowFocus()) if (observer) { - observer.refetch() + observer.refetch({ cancelRefetch: false }) } // Continue fetch if currently paused @@ -308,7 +308,7 @@ export class Query< const observer = this.observers.find(x => x.shouldFetchOnReconnect()) if (observer) { - observer.refetch() + observer.refetch({ cancelRefetch: false }) } // Continue fetch if currently paused From c19eb3f34351a9043cc20bea47e95f7ea8e844d2 Mon Sep 17 00:00:00 2001 From: Dominik Dorfmeister Date: Sat, 13 Nov 2021 14:49:41 +0100 Subject: [PATCH 3/9] feat: streamline cancelRefetch update tests refetch and invalidate now both cancel previous queries, which is intended, so we get more calls to the queryFn in these cases --- src/core/tests/queryClient.test.tsx | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/core/tests/queryClient.test.tsx b/src/core/tests/queryClient.test.tsx index 5e0ee089a1..83fe07b940 100644 --- a/src/core/tests/queryClient.test.tsx +++ b/src/core/tests/queryClient.test.tsx @@ -694,7 +694,8 @@ describe('queryClient', () => { queryClient.invalidateQueries(key1) await queryClient.refetchQueries({ stale: true }) unsubscribe() - expect(queryFn1).toHaveBeenCalledTimes(2) + // fetchQuery, observer mount, invalidation (cancels observer mount) and refetch + expect(queryFn1).toHaveBeenCalledTimes(4) expect(queryFn2).toHaveBeenCalledTimes(1) }) @@ -711,7 +712,10 @@ describe('queryClient', () => { queryFn: queryFn1, }) const unsubscribe = observer.subscribe() - await queryClient.refetchQueries({ active: true, stale: true }) + await queryClient.refetchQueries( + { active: true, stale: true }, + { cancelRefetch: false } + ) unsubscribe() expect(queryFn1).toHaveBeenCalledTimes(2) expect(queryFn2).toHaveBeenCalledTimes(1) From ce67c20f264a172cd23426232d155a63e10f2a95 Mon Sep 17 00:00:00 2001 From: Dominik Dorfmeister Date: Sat, 13 Nov 2021 14:54:57 +0100 Subject: [PATCH 4/9] feat: streamline cancelRefetch add more tests for cancelRefetch behavior --- src/core/tests/queryClient.test.tsx | 34 +++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/src/core/tests/queryClient.test.tsx b/src/core/tests/queryClient.test.tsx index 83fe07b940..7fa76f9e74 100644 --- a/src/core/tests/queryClient.test.tsx +++ b/src/core/tests/queryClient.test.tsx @@ -944,9 +944,10 @@ describe('queryClient', () => { expect(queryFn2).toHaveBeenCalledTimes(1) }) - test('should cancel ongoing fetches if cancelRefetch option is passed', async () => { + test('should cancel ongoing fetches if cancelRefetch option is set (default value)', async () => { const key = queryKey() const cancelFn = jest.fn() + let fetchCount = 0 const observer = new QueryObserver(queryClient, { queryKey: key, enabled: false, @@ -956,6 +957,7 @@ describe('queryClient', () => { queryClient.fetchQuery(key, () => { const promise = new Promise(resolve => { + fetchCount++ setTimeout(() => resolve(5), 10) }) // @ts-expect-error @@ -963,9 +965,37 @@ describe('queryClient', () => { return promise }) - await queryClient.refetchQueries(undefined, { cancelRefetch: true }) + await queryClient.refetchQueries() observer.destroy() expect(cancelFn).toHaveBeenCalledTimes(1) + expect(fetchCount).toBe(2) + }) + + test('should not cancel ongoing fetches if cancelRefetch option is set to false', async () => { + const key = queryKey() + const cancelFn = jest.fn() + let fetchCount = 0 + const observer = new QueryObserver(queryClient, { + queryKey: key, + enabled: false, + initialData: 1, + }) + observer.subscribe() + + queryClient.fetchQuery(key, () => { + const promise = new Promise(resolve => { + fetchCount++ + setTimeout(() => resolve(5), 10) + }) + // @ts-expect-error + promise.cancel = cancelFn + return promise + }) + + await queryClient.refetchQueries(undefined, { cancelRefetch: false }) + observer.destroy() + expect(cancelFn).toHaveBeenCalledTimes(0) + expect(fetchCount).toBe(1) }) }) From c248e49d638ea76b0527db6815ce9d9450487a11 Mon Sep 17 00:00:00 2001 From: Dominik Dorfmeister Date: Sat, 13 Nov 2021 15:14:19 +0100 Subject: [PATCH 5/9] feat: streamline cancelRefetch update docs and migration guide --- .../guides/migrating-to-react-query-4.md | 28 +++++++++++++++++++ docs/src/pages/reference/QueryClient.md | 18 ++++++++---- docs/src/pages/reference/useQuery.md | 5 +++- 3 files changed, 44 insertions(+), 7 deletions(-) diff --git a/docs/src/pages/guides/migrating-to-react-query-4.md b/docs/src/pages/guides/migrating-to-react-query-4.md index 65e54687e1..a62afb5b6b 100644 --- a/docs/src/pages/guides/migrating-to-react-query-4.md +++ b/docs/src/pages/guides/migrating-to-react-query-4.md @@ -14,6 +14,34 @@ With version [3.22.0](https://github.com/tannerlinsley/react-query/releases/tag/ + import { dehydrate, hydrate, useHydrate, Hydrate } from 'react-query' ``` +### Consistent behavior for `cancelRefetch` +The `cancelRefetch` can be passed to all functions that imperatively fetch a query, namely: +- `queryClient.refetchQueries` + - `queryClient.invalidateQueries` + - `queryClient.resetQueries` +- `refetch` returned from `useQuery` +- `fetchNetPage` and `fetchPreviousPage` returned from `useInfiniteQuery` +Except for `fetchNetxPage` and `fetchPreviousPage`, this flag was defaulting to `false`, which was inconsistent and potentially troublesome: Calling `refetchQueries` or `invalidateQueries` after a mutation might not yield the latest result if a previous slow fetch was already ongoing, because this refetch would have been skipped. + +We believe that if a query is actively refetched by some code you write, it should, per default, re-start the fetch. + +That is why this flag now defaults to _true_ for all methods mentioned above. It also means that if you call `refetchQueries` twice in a row, without awaiting it, it will now cancel the first fetch and re-start it with the second one: + +``` +queryClient.refetchQueries({ queryKey: ['todos'] }) +// this will abort the previous refetch and start a new fetch +queryClient.refetchQueries({ queryKey: ['todos'] }) +``` + +You can opt-out of this behaviour by explicitly passing `cancelRefetch:false`: + +``` +queryClient.refetchQueries({ queryKey: ['todos'] }) +// this will not abort the previous refetch - it will just be ignored +queryClient.refetchQueries({ queryKey: ['todos'] }, { cancelRefetch: false }) +``` + +> Note: There is no change in behaviour for automatically triggered fetches, e.g. because a query mounts or because of a window focus refetch. diff --git a/docs/src/pages/reference/QueryClient.md b/docs/src/pages/reference/QueryClient.md index be3fa19d6a..c826b81f21 100644 --- a/docs/src/pages/reference/QueryClient.md +++ b/docs/src/pages/reference/QueryClient.md @@ -295,8 +295,10 @@ await queryClient.invalidateQueries('posts', { - `options?: InvalidateOptions`: - `throwOnError?: boolean` - When set to `true`, this method will throw if any of the query refetch tasks fail. - - cancelRefetch?: boolean - - When set to `true`, then the current request will be cancelled before a new request is made + - `cancelRefetch?: boolean` + - Defaults to `true` + - Per default, a currently running request will be cancelled before a new request is made + - When set to `false`, no refetch will be made if there is already a request running. ## `queryClient.refetchQueries` @@ -328,8 +330,10 @@ await queryClient.refetchQueries(['posts', 1], { active: true, exact: true }) - `options?: RefetchOptions`: - `throwOnError?: boolean` - When set to `true`, this method will throw if any of the query refetch tasks fail. - - cancelRefetch?: boolean - - When set to `true`, then the current request will be cancelled before a new request is made + - `cancelRefetch?: boolean` + - Defaults to `true` + - Per default, a currently running request will be cancelled before a new request is made + - When set to `false`, no refetch will be made if there is already a request running. **Returns** @@ -396,8 +400,10 @@ queryClient.resetQueries(queryKey, { exact: true }) - `options?: ResetOptions`: - `throwOnError?: boolean` - When set to `true`, this method will throw if any of the query refetch tasks fail. - - cancelRefetch?: boolean - - When set to `true`, then the current request will be cancelled before a new request is made + - `cancelRefetch?: boolean` + - Defaults to `true` + - Per default, a currently running request will be cancelled before a new request is made + - When set to `false`, no refetch will be made if there is already a request running. **Returns** diff --git a/docs/src/pages/reference/useQuery.md b/docs/src/pages/reference/useQuery.md index 0b5588acc7..a2464cd2b7 100644 --- a/docs/src/pages/reference/useQuery.md +++ b/docs/src/pages/reference/useQuery.md @@ -236,6 +236,9 @@ const result = useQuery({ - `refetch: (options: { throwOnError: boolean, cancelRefetch: boolean }) => Promise` - A function to manually refetch the query. - If the query errors, the error will only be logged. If you want an error to be thrown, pass the `throwOnError: true` option - - If `cancelRefetch` is `true`, then the current request will be cancelled before a new request is made + - `cancelRefetch?: boolean` + - Defaults to `true` + - Per default, a currently running request will be cancelled before a new request is made + - When set to `false`, no refetch will be made if there is already a request running. - `remove: () => void` - A function to remove the query from the cache. From 8ec3cc31166939bb93a3b8cbc3c2e9bc2daaf303 Mon Sep 17 00:00:00 2001 From: Dominik Dorfmeister Date: Sat, 13 Nov 2021 16:17:54 +0100 Subject: [PATCH 6/9] feat: streamline cancelRefetch simplify conditions by moving the ?? true default down to fetch on observer level; all 3 callers (fetchNextPage, fetchPreviousPage and refetch) just pass their options down and adhere to this default; refetch also only has 3 callers: - refetch from useQuery, where we want the default - onOnline and onFocus, where we now explicitly pass false to keep the previous behavior and add more tests --- src/core/infiniteQueryObserver.ts | 25 ++++---- src/core/queryObserver.ts | 6 +- src/react/tests/useQuery.test.tsx | 102 ++++++++++++++++++++++++++++++ 3 files changed, 119 insertions(+), 14 deletions(-) diff --git a/src/core/infiniteQueryObserver.ts b/src/core/infiniteQueryObserver.ts index fc76ded7db..f2c6c9365a 100644 --- a/src/core/infiniteQueryObserver.ts +++ b/src/core/infiniteQueryObserver.ts @@ -90,26 +90,27 @@ export class InfiniteQueryObserver< > } - fetchNextPage( - options?: FetchNextPageOptions - ): Promise> { + fetchNextPage({ pageParam, ...options }: FetchNextPageOptions = {}): Promise< + InfiniteQueryObserverResult + > { return this.fetch({ - cancelRefetch: options?.cancelRefetch ?? true, - throwOnError: options?.throwOnError, + ...options, meta: { - fetchMore: { direction: 'forward', pageParam: options?.pageParam }, + fetchMore: { direction: 'forward', pageParam }, }, }) } - fetchPreviousPage( - options?: FetchPreviousPageOptions - ): Promise> { + fetchPreviousPage({ + pageParam, + ...options + }: FetchPreviousPageOptions = {}): Promise< + InfiniteQueryObserverResult + > { return this.fetch({ - cancelRefetch: options?.cancelRefetch ?? true, - throwOnError: options?.throwOnError, + ...options, meta: { - fetchMore: { direction: 'backward', pageParam: options?.pageParam }, + fetchMore: { direction: 'backward', pageParam }, }, }) } diff --git a/src/core/queryObserver.ts b/src/core/queryObserver.ts index b43c0681e1..4adc527cb0 100644 --- a/src/core/queryObserver.ts +++ b/src/core/queryObserver.ts @@ -283,7 +283,6 @@ export class QueryObserver< ): Promise> { return this.fetch({ ...options, - cancelRefetch: options?.cancelRefetch ?? true, meta: { refetchPage: options?.refetchPage }, }) } @@ -317,7 +316,10 @@ export class QueryObserver< protected fetch( fetchOptions?: ObserverFetchOptions ): Promise> { - return this.executeFetch(fetchOptions).then(() => { + return this.executeFetch({ + ...fetchOptions, + cancelRefetch: fetchOptions?.cancelRefetch ?? true, + }).then(() => { this.updateResult() return this.currentResult }) diff --git a/src/react/tests/useQuery.test.tsx b/src/react/tests/useQuery.test.tsx index 547d5ddf1b..c0a9a77584 100644 --- a/src/react/tests/useQuery.test.tsx +++ b/src/react/tests/useQuery.test.tsx @@ -549,6 +549,108 @@ describe('useQuery', () => { consoleMock.mockRestore() }) + it('should not cancel an ongoing fetch when refetch is called with cancelRefetch=false if we have data already', async () => { + const key = queryKey() + let fetchCount = 0 + + function Page() { + const { refetch } = useQuery( + key, + async () => { + fetchCount++ + await sleep(10) + return 'data' + }, + { enabled: false, initialData: 'initialData' } + ) + + React.useEffect(() => { + setActTimeout(() => { + refetch() + }, 5) + setActTimeout(() => { + refetch({ cancelRefetch: false }) + }, 5) + }, [refetch]) + + return null + } + + renderWithClient(queryClient, ) + + await sleep(20) + // first refetch only, second refetch is ignored + expect(fetchCount).toBe(1) + }) + + it('should cancel an ongoing fetch when refetch is called (cancelRefetch=true) if we have data already', async () => { + const key = queryKey() + let fetchCount = 0 + + function Page() { + const { refetch } = useQuery( + key, + async () => { + fetchCount++ + await sleep(10) + return 'data' + }, + { enabled: false, initialData: 'initialData' } + ) + + React.useEffect(() => { + setActTimeout(() => { + refetch() + }, 5) + setActTimeout(() => { + refetch() + }, 5) + }, [refetch]) + + return null + } + + renderWithClient(queryClient, ) + + await sleep(20) + // first refetch (gets cancelled) and second refetch + expect(fetchCount).toBe(2) + }) + + it('should not cancel an ongoing fetch when refetch is called (cancelRefetch=true) if we do not have data yet', async () => { + const key = queryKey() + let fetchCount = 0 + + function Page() { + const { refetch } = useQuery( + key, + async () => { + fetchCount++ + await sleep(10) + return 'data' + }, + { enabled: false } + ) + + React.useEffect(() => { + setActTimeout(() => { + refetch() + }, 5) + setActTimeout(() => { + refetch() + }, 5) + }, [refetch]) + + return null + } + + renderWithClient(queryClient, ) + + await sleep(20) + // first refetch will not get cancelled, second one gets skipped + expect(fetchCount).toBe(1) + }) + it('should be able to watch a query without providing a query function', async () => { const key = queryKey() const states: UseQueryResult[] = [] From e7013f311da1fb68801e1186e8cbbc33f30a202e Mon Sep 17 00:00:00 2001 From: Dominik Dorfmeister Date: Sat, 13 Nov 2021 16:24:17 +0100 Subject: [PATCH 7/9] feat: streamline cancelRefetch we always call this.fetch() with options, so we can just as well make the mandatory also, streamline signatures by destructing values that can't be forwarded (and use empty object as default value) in options and just spread the rest --- src/core/infiniteQueryObserver.ts | 2 +- src/core/queryObserver.ts | 15 +++++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/core/infiniteQueryObserver.ts b/src/core/infiniteQueryObserver.ts index f2c6c9365a..54d0833a7c 100644 --- a/src/core/infiniteQueryObserver.ts +++ b/src/core/infiniteQueryObserver.ts @@ -39,7 +39,7 @@ export class InfiniteQueryObserver< // Type override protected fetch!: ( - fetchOptions?: ObserverFetchOptions + fetchOptions: ObserverFetchOptions ) => Promise> // eslint-disable-next-line @typescript-eslint/no-useless-constructor diff --git a/src/core/queryObserver.ts b/src/core/queryObserver.ts index 4adc527cb0..35d843d845 100644 --- a/src/core/queryObserver.ts +++ b/src/core/queryObserver.ts @@ -278,12 +278,15 @@ export class QueryObserver< this.client.getQueryCache().remove(this.currentQuery) } - refetch( - options?: RefetchOptions & RefetchQueryFilters - ): Promise> { + refetch({ + refetchPage, + ...options + }: RefetchOptions & RefetchQueryFilters = {}): Promise< + QueryObserverResult + > { return this.fetch({ ...options, - meta: { refetchPage: options?.refetchPage }, + meta: { refetchPage }, }) } @@ -314,11 +317,11 @@ export class QueryObserver< } protected fetch( - fetchOptions?: ObserverFetchOptions + fetchOptions: ObserverFetchOptions ): Promise> { return this.executeFetch({ ...fetchOptions, - cancelRefetch: fetchOptions?.cancelRefetch ?? true, + cancelRefetch: fetchOptions.cancelRefetch ?? true, }).then(() => { this.updateResult() return this.currentResult From 8af0081443fdc156d70e2c75caf6c76c52217750 Mon Sep 17 00:00:00 2001 From: Dominik Dorfmeister Date: Sat, 13 Nov 2021 16:28:38 +0100 Subject: [PATCH 8/9] feat: streamline cancelRefetch fix types for refetch it was accidentally made too wide and allowed all refetchFilters, like `predicate`; but with `refetch` on an obserserver, there is nothing to filter for, except the page, so that is what we need to accept via `RefetchPageFilters` --- src/core/queryObserver.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/queryObserver.ts b/src/core/queryObserver.ts index 35d843d845..fe9cba01aa 100644 --- a/src/core/queryObserver.ts +++ b/src/core/queryObserver.ts @@ -1,4 +1,4 @@ -import { RefetchQueryFilters } from './types' +import { RefetchPageFilters } from './types' import { isServer, isValidTimeout, @@ -281,7 +281,7 @@ export class QueryObserver< refetch({ refetchPage, ...options - }: RefetchOptions & RefetchQueryFilters = {}): Promise< + }: RefetchOptions & RefetchPageFilters = {}): Promise< QueryObserverResult > { return this.fetch({ From 9048c894250a754f37a746eefecf8defc5e8bff1 Mon Sep 17 00:00:00 2001 From: Dominik Dorfmeister Date: Sat, 13 Nov 2021 16:33:24 +0100 Subject: [PATCH 9/9] feat: streamline cancelRefetch refetch never took a queryKey as param - it is always bound to the observer --- src/core/tests/queryObserver.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/tests/queryObserver.test.tsx b/src/core/tests/queryObserver.test.tsx index 1cba155cbd..aea4dbc46a 100644 --- a/src/core/tests/queryObserver.test.tsx +++ b/src/core/tests/queryObserver.test.tsx @@ -622,7 +622,7 @@ describe('queryObserver', () => { select: () => selectedData, }) - await observer.refetch({ queryKey: key }) + await observer.refetch() expect(observer.getCurrentResult().data).toBe(selectedData) unsubscribe()