From ae1ea678c538ab2ea13a4976ade122de02a45942 Mon Sep 17 00:00:00 2001 From: Niek Date: Mon, 31 Aug 2020 18:07:31 +0200 Subject: [PATCH] fix: always use config from last query execution --- src/core/query.ts | 261 ++++++++++++++++------------- src/core/queryCache.ts | 86 ++++++---- src/core/queryObserver.ts | 14 +- src/core/tests/queryCache.test.tsx | 16 +- src/hydration/hydration.ts | 6 +- src/react/tests/ssr.test.tsx | 6 +- src/react/tests/useQuery.test.tsx | 24 +++ 7 files changed, 242 insertions(+), 171 deletions(-) diff --git a/src/core/query.ts b/src/core/query.ts index eaad23a613..578cf66909 100644 --- a/src/core/query.ts +++ b/src/core/query.ts @@ -104,6 +104,7 @@ export class Query { config: QueryConfig observers: QueryObserver[] state: QueryState + cacheTime: number private queryCache: QueryCache private promise?: Promise @@ -121,11 +122,13 @@ export class Query { this.notifyGlobalListeners = init.notifyGlobalListeners this.observers = [] this.state = getDefaultState(init.config) + this.cacheTime = init.config.cacheTime! this.scheduleGc() } - updateConfig(config: QueryConfig): void { + private updateConfig(config: QueryConfig): void { this.config = config + this.cacheTime = Math.max(this.cacheTime, config.cacheTime || 0) } private dispatch(action: Action): void { @@ -141,18 +144,21 @@ export class Query { this.clearGcTimeout() - if (this.config.cacheTime === Infinity || this.observers.length > 0) { + if (this.cacheTime === Infinity || this.observers.length > 0) { return } this.gcTimeout = setTimeout(() => { this.clear() - }, this.config.cacheTime) + }, this.cacheTime) } - async refetch(options?: RefetchOptions): Promise { + async refetch( + options?: RefetchOptions, + config?: QueryConfig + ): Promise { try { - return await this.fetch() + return await this.fetch(undefined, config) } catch (error) { if (options?.throwOnError === true) { throw error @@ -210,11 +216,13 @@ export class Query { } clear(): void { + this.queryCache.removeQuery(this) + } + + destroy(): void { this.clearGcTimeout() this.clearTimersObservers() this.cancel() - delete this.queryCache.queries[this.queryHash] - this.notifyGlobalListeners(this) } isEnabled(): boolean { @@ -294,119 +302,21 @@ export class Query { this.scheduleGc() } - private async tryFetchData( - config: QueryConfig, - fn: QueryFunction - ): Promise { - return new Promise((outerResolve, outerReject) => { - let resolved = false - let continueLoop: () => void - let cancelTransport: () => void - - const done = () => { - resolved = true - - delete this.cancelFetch - delete this.continueFetch - delete this.isTransportCancelable - - // End loop if currently paused - continueLoop?.() - } - - const resolve = (value: any) => { - done() - outerResolve(value) - } - - const reject = (value: any) => { - done() - outerReject(value) - } - - // Create callback to cancel this fetch - this.cancelFetch = () => { - reject(new CancelledError()) - try { - cancelTransport?.() - } catch {} - } - - // Create callback to continue this fetch - this.continueFetch = () => { - continueLoop?.() - } - - // Create loop function - const run = async () => { - try { - // Execute query - const promiseOrValue = fn() - - // Check if the transport layer support cancellation - if (isCancelable(promiseOrValue)) { - cancelTransport = () => { - promiseOrValue.cancel() - } - this.isTransportCancelable = true - } - - // Await data - resolve(await promiseOrValue) - } catch (error) { - // Stop if the fetch is already resolved - if (resolved) { - return - } - - // Do we need to retry the request? - const { failureCount } = this.state - const { retry, retryDelay } = config - - const shouldRetry = - retry === true || - failureCount < retry! || - (typeof retry === 'function' && retry(failureCount, error)) - - if (!shouldRetry) { - // We are done if the query does not need to be retried - reject(error) - return - } - - // Increase the failureCount - this.dispatch({ type: ActionType.Failed }) - - // Delay - await sleep(functionalUpdate(retryDelay, failureCount) || 0) - - // Pause retry if the document is not visible or when the device is offline - if (!isDocumentVisible() || !isOnline()) { - await new Promise(continueResolve => { - continueLoop = continueResolve - }) - } - - // Try again if not resolved yet - if (!resolved) { - run() - } - } - } - - // Start loop - run() - }) - } - - async fetch(options?: FetchOptions): Promise { + async fetch( + options?: FetchOptions, + config?: QueryConfig + ): Promise { // If we are already fetching, return current promise if (this.promise) { return this.promise } - // Store reference to the config that initiated this fetch - const config = this.config + // Update config if passed, otherwise the config from the last execution is used + if (config) { + this.updateConfig(config) + } + + config = this.config // Check if there is a query function if (!config.queryFn) { @@ -530,16 +440,125 @@ export class Query { return this.tryFetchData(config, fetchData) } + private async tryFetchData( + config: QueryConfig, + fn: QueryFunction + ): Promise { + return new Promise((outerResolve, outerReject) => { + let resolved = false + let continueLoop: () => void + let cancelTransport: () => void + + const done = () => { + resolved = true + + delete this.cancelFetch + delete this.continueFetch + delete this.isTransportCancelable + + // End loop if currently paused + continueLoop?.() + } + + const resolve = (value: any) => { + done() + outerResolve(value) + } + + const reject = (value: any) => { + done() + outerReject(value) + } + + // Create callback to cancel this fetch + this.cancelFetch = () => { + reject(new CancelledError()) + try { + cancelTransport?.() + } catch {} + } + + // Create callback to continue this fetch + this.continueFetch = () => { + continueLoop?.() + } + + // Create loop function + const run = async () => { + try { + // Execute query + const promiseOrValue = fn() + + // Check if the transport layer support cancellation + if (isCancelable(promiseOrValue)) { + cancelTransport = () => { + promiseOrValue.cancel() + } + this.isTransportCancelable = true + } + + // Await data + resolve(await promiseOrValue) + } catch (error) { + // Stop if the fetch is already resolved + if (resolved) { + return + } + + // Do we need to retry the request? + const { failureCount } = this.state + const { retry, retryDelay } = config + + const shouldRetry = + retry === true || + failureCount < retry! || + (typeof retry === 'function' && retry(failureCount, error)) + + if (!shouldRetry) { + // We are done if the query does not need to be retried + reject(error) + return + } + + // Increase the failureCount + this.dispatch({ type: ActionType.Failed }) + + // Delay + await sleep(functionalUpdate(retryDelay, failureCount) || 0) + + // Pause retry if the document is not visible or when the device is offline + if (!isDocumentVisible() || !isOnline()) { + await new Promise(continueResolve => { + continueLoop = continueResolve + }) + } + + // Try again if not resolved yet + if (!resolved) { + run() + } + } + } + + // Start loop + run() + }) + } + fetchMore( fetchMoreVariable?: unknown, - options?: FetchMoreOptions + options?: FetchMoreOptions, + config?: QueryConfig ): Promise { - return this.fetch({ - fetchMore: { - fetchMoreVariable, - previous: options?.previous || false, + return this.fetch( + { + fetchMore: { + fetchMoreVariable, + previous: options?.previous || false, + }, }, - }) + config + ) } } diff --git a/src/core/queryCache.ts b/src/core/queryCache.ts index 3c8c63dde1..4f737361a8 100644 --- a/src/core/queryCache.ts +++ b/src/core/queryCache.ts @@ -68,11 +68,12 @@ type QueryCacheListener = ( // CLASS export class QueryCache { - queries: QueryHashMap isFetching: number private config: QueryCacheConfig private globalListeners: QueryCacheListener[] + private queries: QueryHashMap + private queriesArray: Query[] constructor(config?: QueryCacheConfig) { this.config = config || {} @@ -81,11 +82,12 @@ export class QueryCache { this.globalListeners = [] this.queries = {} + this.queriesArray = [] this.isFetching = 0 } private notifyGlobalListeners(query?: Query) { - this.isFetching = Object.values(this.queries).reduce( + this.isFetching = this.getQueries().reduce( (acc, query) => (query.state.isFetching ? acc + 1 : acc), 0 ) @@ -108,24 +110,23 @@ export class QueryCache { subscribe(listener: QueryCacheListener): () => void { this.globalListeners.push(listener) return () => { - this.globalListeners.splice(this.globalListeners.indexOf(listener), 1) + this.globalListeners = this.globalListeners.filter(x => x !== listener) } } clear(options?: ClearOptions): void { - Object.values(this.queries).forEach(query => query.clear()) - this.queries = {} + this.removeQueries() if (options?.notify) { this.notifyGlobalListeners() } } getQueries( - predicate: QueryPredicate, + predicate?: QueryPredicate, options?: QueryPredicateOptions ): Query[] { - if (predicate === true) { - return Object.values(this.queries) + if (predicate === true || typeof predicate === 'undefined') { + return this.queriesArray } let predicateFn: QueryPredicateFn @@ -142,7 +143,7 @@ export class QueryCache { : deepIncludes(d.queryKey, queryKey) } - return Object.values(this.queries).filter(predicateFn) + return this.queriesArray.filter(predicateFn) } getQuery( @@ -155,22 +156,35 @@ export class QueryCache { return this.getQuery(predicate)?.state.data } + removeQuery(query: Query): void { + if (this.queries[query.queryHash]) { + query.destroy() + delete this.queries[query.queryHash] + this.queriesArray = this.queriesArray.filter(x => x !== query) + this.notifyGlobalListeners(query) + } + } + removeQueries( - predicate: QueryPredicate, + predicate?: QueryPredicate, options?: QueryPredicateOptions ): void { - this.getQueries(predicate, options).forEach(query => query.clear()) + this.getQueries(predicate, options).forEach(query => { + this.removeQuery(query) + }) } cancelQueries( - predicate: QueryPredicate, + predicate?: QueryPredicate, options?: QueryPredicateOptions ): void { - this.getQueries(predicate, options).forEach(query => query.cancel()) + this.getQueries(predicate, options).forEach(query => { + query.cancel() + }) } async invalidateQueries( - predicate: QueryPredicate, + predicate?: QueryPredicate, options?: InvalidateQueriesOptions ): Promise { const { refetchActive = true, refetchInactive = false, throwOnError } = @@ -200,7 +214,7 @@ export class QueryCache { } resetErrorBoundaries(): void { - this.getQueries(true).forEach(query => { + this.getQueries().forEach(query => { query.state.throwInErrorBoundary = false }) } @@ -210,31 +224,26 @@ export class QueryCache { queryConfig?: QueryConfig ): Query { const config = this.getDefaultedQueryConfig(queryConfig) - const [queryHash, queryKey] = config.queryKeySerializerFn!(userQueryKey) - let query - if (this.queries[queryHash]) { - query = this.queries[queryHash] as Query - query.updateConfig(config) + return this.queries[queryHash] as Query } - if (!query) { - query = new Query({ - queryCache: this, - queryKey, - queryHash, - config, - notifyGlobalListeners: query => { - this.notifyGlobalListeners(query) - }, - }) - - if (!this.config.frozen) { - this.queries[queryHash] = query + const query = new Query({ + queryCache: this, + queryKey, + queryHash, + config, + notifyGlobalListeners: query => { this.notifyGlobalListeners(query) - } + }, + }) + + if (!this.config.frozen) { + this.queries[queryHash] = query + this.queriesArray.push(query) + this.notifyGlobalListeners(query) } return query @@ -300,13 +309,16 @@ export class QueryCache { >(args) // https://github.com/tannerlinsley/react-query/issues/652 - const configWithoutRetry = { retry: false, ...config } + const configWithoutRetry = this.getDefaultedQueryConfig({ + retry: false, + ...config, + }) let query try { query = this.buildQuery(queryKey, configWithoutRetry) if (options?.force || query.isStaleByTime(config.staleTime)) { - await query.fetch() + await query.fetch(undefined, configWithoutRetry) } return query.state.data } catch (error) { @@ -348,7 +360,7 @@ export function makeQueryCache(config?: QueryCacheConfig) { export function onVisibilityOrOnlineChange(isOnlineChange: boolean) { if (isDocumentVisible() && isOnline()) { queryCaches.forEach(queryCache => { - queryCache.getQueries(query => { + queryCache.getQueries().forEach(query => { if (isOnlineChange) { query.onOnline() } else { diff --git a/src/core/queryObserver.ts b/src/core/queryObserver.ts index 505308ed0c..8fcda590f9 100644 --- a/src/core/queryObserver.ts +++ b/src/core/queryObserver.ts @@ -7,6 +7,7 @@ import type { FetchMoreOptions, RefetchOptions, } from './query' +import type { QueryCache } from './queryCache' export type UpdateListener = ( result: QueryResult @@ -15,6 +16,7 @@ export type UpdateListener = ( export class QueryObserver { config: QueryObserverConfig + private queryCache: QueryCache private currentQuery!: Query private currentResult!: QueryResult private previousResult?: QueryResult @@ -25,6 +27,7 @@ export class QueryObserver { constructor(config: QueryObserverConfig) { this.config = config + this.queryCache = config.queryCache! // Bind exposed methods this.clear = this.clear.bind(this) @@ -106,22 +109,19 @@ export class QueryObserver { } async refetch(options?: RefetchOptions): Promise { - this.currentQuery.updateConfig(this.config) - return this.currentQuery.refetch(options) + return this.currentQuery.refetch(options, this.config) } async fetchMore( fetchMoreVariable?: unknown, options?: FetchMoreOptions ): Promise { - this.currentQuery.updateConfig(this.config) - return this.currentQuery.fetchMore(fetchMoreVariable, options) + return this.currentQuery.fetchMore(fetchMoreVariable, options, this.config) } async fetch(): Promise { - this.currentQuery.updateConfig(this.config) try { - return await this.currentQuery.fetch() + return await this.currentQuery.fetch(undefined, this.config) } catch (error) { return undefined } @@ -281,7 +281,7 @@ export class QueryObserver { ? { ...this.config, initialData: undefined } : this.config - const newQuery = config.queryCache!.buildQuery(config.queryKey, config) + const newQuery = this.queryCache.buildQuery(config.queryKey, config) if (newQuery === prevQuery) { return false diff --git a/src/core/tests/queryCache.test.tsx b/src/core/tests/queryCache.test.tsx index 1a7df97add..4cb3d5018b 100644 --- a/src/core/tests/queryCache.test.tsx +++ b/src/core/tests/queryCache.test.tsx @@ -267,7 +267,6 @@ describe('queryCache', () => { }) const query = defaultQueryCache.getQuery(key)! const instance = query.subscribe() - instance.updateConfig(query.config) // @ts-expect-error expect(instance.refetchIntervalId).not.toBeUndefined() instance.unsubscribe() @@ -362,6 +361,21 @@ describe('queryCache', () => { unsubscribe() }) + test('query should use the longest cache time it has seen', async () => { + const key = queryKey() + await defaultQueryCache.prefetchQuery(key, () => 'data', { + cacheTime: 100, + }) + await defaultQueryCache.prefetchQuery(key, () => 'data', { + cacheTime: 200, + }) + await defaultQueryCache.prefetchQuery(key, () => 'data', { + cacheTime: 10, + }) + const query = defaultQueryCache.getQuery(key)! + expect(query.cacheTime).toBe(200) + }) + it('should continue retry after focus regain and resolve all promises', async () => { const key = queryKey() diff --git a/src/hydration/hydration.ts b/src/hydration/hydration.ts index a3c41ba334..64b424230d 100644 --- a/src/hydration/hydration.ts +++ b/src/hydration/hydration.ts @@ -40,8 +40,8 @@ function dehydrateQuery( // in the html-payload, but not consume it on the initial render. // We still schedule stale and garbage collection right away, which means // we need to specifically include staleTime and cacheTime in dehydration. - if (query.config.cacheTime !== DEFAULT_CACHE_TIME) { - dehydratedQuery.config.cacheTime = query.config.cacheTime + if (query.cacheTime !== DEFAULT_CACHE_TIME) { + dehydratedQuery.config.cacheTime = query.cacheTime } if (query.state.data !== undefined) { dehydratedQuery.config.initialData = query.state.data @@ -62,7 +62,7 @@ export function dehydrate( const dehydratedState: DehydratedState = { queries: [], } - for (const query of Object.values(queryCache.queries)) { + for (const query of queryCache.getQueries()) { if (shouldDehydrate(query)) { dehydratedState.queries.push(dehydrateQuery(query)) } diff --git a/src/react/tests/ssr.test.tsx b/src/react/tests/ssr.test.tsx index 2c2f67b826..68f4b1aff4 100644 --- a/src/react/tests/ssr.test.tsx +++ b/src/react/tests/ssr.test.tsx @@ -96,7 +96,7 @@ describe('Server Side Rendering', () => { renderToString() - expect(queryCache.queries).toEqual({}) + expect(queryCache.getQueries().length).toEqual(0) }) it('should not add prefetched data to the cache', async () => { @@ -209,7 +209,9 @@ describe('Server Side Rendering', () => { ) - expect(Object.keys(queryCache.queries)).toEqual([`["${key}",1]`]) + const keys = queryCache.getQueries().map(query => query.queryHash) + + expect(keys).toEqual([`["${key}",1]`]) }) it('should not call setTimeout', async () => { diff --git a/src/react/tests/useQuery.test.tsx b/src/react/tests/useQuery.test.tsx index 2307b4efe9..3f82c10e88 100644 --- a/src/react/tests/useQuery.test.tsx +++ b/src/react/tests/useQuery.test.tsx @@ -577,6 +577,30 @@ describe('useQuery', () => { rendered.getByText('Second Status: success') }) + it('should not override query configuration on render', async () => { + const key = queryKey() + + const queryFn1 = async () => { + await sleep(10) + return 'data1' + } + + const queryFn2 = async () => { + await sleep(10) + return 'data2' + } + + function Page() { + useQuery(key, queryFn1) + useQuery(key, queryFn2) + return null + } + + render() + + expect(queryCache.getQuery(key)!.config.queryFn).toBe(queryFn1) + }) + // See https://github.com/tannerlinsley/react-query/issues/170 it('should start with status idle if enabled is false', async () => { const key1 = queryKey()