Feature/cachetime zero (#3054)

* refactor: cacheTime-zero

remove special handling for cacheTime: 0 and schedule a normal garbage collection for those queries. They will be eligible for gc after a setTimeout(0), but then they will only be optionally removed. This makes sure that paused queries are NOT gc'ed

* refactor: cacheTime-zero

remove special test "about online queries with cacheTime:0 should not fetch if paused and then unmounted". paused queries will now be kept until they continue, just like with every other query, unless query cancellation or abort signal was involved

* refactor: cacheTime-zero

adapt "remounting" test: if the same query with cacheTime 0 unmounts and remounts in the same cycle, the query will now be picked up and will not go to loading state again. I think this is okay

* refactor: cacheTime-zero

re-add instant query removal after fetching, because fetching via `queryClient.fetchQuery` will not remove the query otherwise, because the normal gc-mechanism now checks for `hadObservers` due to a suspense issue :/

* refactor: cacheTime-zero

weird edge case: the previous logic was instantly removing the query _while_ it was still fetching, which is something we likely don't want. The data will stay in the currentQuery of the observer if the observer unsubscribes but still exists, and a new subscription will pick it up, unless the query was explicitly cancelled or the abort signal was consumed.

* refactor: cacheTime-zero

we need to wait a tick because even cacheTime 0 now waits at least a setTimeout(0) to be eligible for gc

* refactor: cacheTime-zero

schedule a new garbage collection after each new fetch; this won't do anything  when you still have observers, but it fixes an edge case where prefetching took longer than the cacheTime, in which case the query was again never removed

test needed adaption because we don't instantly remove, but deferred by a tick

* refactor: cacheTime-zero

stabilize test

* refactor: cacheTime-zero

apply a different suspense "workaround": do not garbage collect when fetching optimistically (done only by suspense) - gc will kick in once an observer subscribes; this will make sure we can still gc other fetches that don't have an observer consistently, like prefetching when the fetch takes longer than the gc time (which was leaking with the old workaround)

* refactor: cacheTime-zero

remove leftover

* refactor: cacheTime-zero

since every fetch triggers a new gc cycle, we don't need to do this in a loop anymore
also, reset isFetchingOptimistic after every fetch

Author: TkDodo

src/core/mutation.ts

@@ -131,11 +131,7 @@ export class Mutation<
   removeObserver(observer: MutationObserver<any, any, any, any>): void {
     this.observers = this.observers.filter(x => x !== observer)
 
-    if (this.cacheTime) {
-      this.scheduleGc()
-    } else {
-      this.optionalRemove()
-    }
+    this.scheduleGc()
 
     this.mutationCache.notify({
       type: 'observerRemoved',

src/core/query.ts

@@ -154,20 +154,19 @@ export class Query<
   revertState?: QueryState<TData, TError>
   state: QueryState<TData, TError>
   meta: QueryMeta | undefined
+  isFetchingOptimistic?: boolean
 
   private cache: QueryCache
   private promise?: Promise<TData>
   private retryer?: Retryer<TData, TError>
   private observers: QueryObserver<any, any, any, any, any>[]
   private defaultOptions?: QueryOptions<TQueryFnData, TError, TData, TQueryKey>
   private abortSignalConsumed: boolean
-  private hadObservers: boolean
 
   constructor(config: QueryConfig<TQueryFnData, TError, TData, TQueryKey>) {
     super()
 
     this.abortSignalConsumed = false
-    this.hadObservers = false
     this.defaultOptions = config.defaultOptions
     this.setOptions(config.options)
     this.observers = []
@@ -177,7 +176,6 @@ export class Query<
     this.initialState = config.state || this.getDefaultState(this.options)
     this.state = this.initialState
     this.meta = config.meta
-    this.scheduleGc()
   }
 
   private setOptions(
@@ -197,12 +195,8 @@ export class Query<
   }
 
   protected optionalRemove() {
-    if (!this.observers.length) {
-      if (this.state.fetchStatus === 'idle') {
-        this.cache.remove(this)
-      } else if (this.hadObservers) {
-        this.scheduleGc()
-      }
+    if (!this.observers.length && this.state.fetchStatus === 'idle') {
+      this.cache.remove(this)
     }
   }
 
@@ -307,7 +301,6 @@ export class Query<
   addObserver(observer: QueryObserver<any, any, any, any, any>): void {
     if (this.observers.indexOf(observer) === -1) {
       this.observers.push(observer)
-      this.hadObservers = true
 
       // Stop the query from being garbage collected
       this.clearGcTimeout()
@@ -331,11 +324,7 @@ export class Query<
           }
         }
 
-        if (this.cacheTime) {
-          this.scheduleGc()
-        } else {
-          this.cache.remove(this)
-        }
+        this.scheduleGc()
       }
 
       this.cache.notify({ type: 'observerRemoved', query: this, observer })
@@ -455,10 +444,11 @@ export class Query<
         // Notify cache callback
         this.cache.config.onSuccess?.(data, this as Query<any, any, any, any>)
 
-        // Remove query after fetching if cache time is 0
-        if (this.cacheTime === 0) {
-          this.optionalRemove()
+        if (!this.isFetchingOptimistic) {
+          // Schedule query gc after fetching
+          this.scheduleGc()
         }
+        this.isFetchingOptimistic = false
       },
       onError: (error: TError | { silent?: boolean }) => {
         // Optimistically update state if needed
@@ -477,10 +467,11 @@ export class Query<
           getLogger().error(error)
         }
 
-        // Remove query after fetching if cache time is 0
-        if (this.cacheTime === 0) {
-          this.optionalRemove()
+        if (!this.isFetchingOptimistic) {
+          // Schedule query gc after fetching
+          this.scheduleGc()
         }
+        this.isFetchingOptimistic = false
       },
       onFail: () => {
         this.dispatch({ type: 'failed' })

src/core/queryObserver.ts

@@ -309,15 +309,8 @@ export class QueryObserver<
 
     const query = this.client
       .getQueryCache()
-      .build(
-        this.client,
-        defaultedOptions as QueryOptions<
-          TQueryFnData,
-          TError,
-          TQueryData,
-          TQueryKey
-        >
-      )
+      .build(this.client, defaultedOptions)
+    query.isFetchingOptimistic = true
 
     return query.fetch().then(() => this.createResult(query, defaultedOptions))
   }
@@ -655,17 +648,7 @@ export class QueryObserver<
   }
 
   private updateQuery(): void {
-    const query = this.client
-      .getQueryCache()
-      .build(
-        this.client,
-        this.options as QueryOptions<
-          TQueryFnData,
-          TError,
-          TQueryData,
-          TQueryKey
-        >
-      )
+    const query = this.client.getQueryCache().build(this.client, this.options)
 
     if (query === this.currentQuery) {
       return

src/core/tests/query.test.tsx

@@ -13,6 +13,7 @@ import {
   onlineManager,
   QueryFunctionContext,
 } from '../..'
+import { waitFor } from '@testing-library/react'
 
 describe('query', () => {
   let queryClient: QueryClient
@@ -472,7 +473,6 @@ describe('query', () => {
   })
 
   test('queries with cacheTime 0 should be removed immediately after unsubscribing', async () => {
-    const consoleMock = mockConsoleError()
     const key = queryKey()
     let count = 0
     const observer = new QueryObserver(queryClient, {
@@ -486,13 +486,12 @@ describe('query', () => {
     })
     const unsubscribe1 = observer.subscribe()
     unsubscribe1()
-    await sleep(10)
+    await waitFor(() => expect(queryCache.find(key)).toBeUndefined())
     const unsubscribe2 = observer.subscribe()
     unsubscribe2()
-    await sleep(10)
-    expect(count).toBe(2)
-    expect(queryCache.find(key)).toBeUndefined()
-    consoleMock.mockRestore()
+
+    await waitFor(() => expect(queryCache.find(key)).toBeUndefined())
+    expect(count).toBe(1)
   })
 
   test('should be garbage collected when unsubscribed to', async () => {
@@ -506,7 +505,7 @@ describe('query', () => {
     const unsubscribe = observer.subscribe()
     expect(queryCache.find(key)).toBeDefined()
     unsubscribe()
-    expect(queryCache.find(key)).toBeUndefined()
+    await waitFor(() => expect(queryCache.find(key)).toBeUndefined())
   })
 
   test('should be garbage collected later when unsubscribed and query is fetching', async () => {
@@ -529,7 +528,7 @@ describe('query', () => {
     expect(queryCache.find(key)).toBeDefined()
     await sleep(10)
     // should be removed after an additional staleTime wait
-    expect(queryCache.find(key)).toBeUndefined()
+    await waitFor(() => expect(queryCache.find(key)).toBeUndefined())
   })
 
   test('should not be garbage collected unless there are no subscribers', async () => {

src/core/tests/queryClient.test.tsx

@@ -416,9 +416,10 @@ describe('queryClient', () => {
         },
         { cacheTime: 0 }
       )
-      const result2 = queryClient.getQueryData(key1)
       expect(result).toEqual(1)
-      expect(result2).toEqual(undefined)
+      await waitFor(() =>
+        expect(queryClient.getQueryData(key1)).toEqual(undefined)
+      )
     })
 
     test('should keep a query in cache if cache time is Infinity', async () => {

src/reactjs/tests/useQuery.test.tsx

@@ -691,52 +691,78 @@ describe('useQuery', () => {
     expect(states[1]).toMatchObject({ data: 'data' })
   })
 
-  it('should create a new query when re-mounting with cacheTime 0', async () => {
+  it('should pick up a query when re-mounting with cacheTime 0', async () => {
     const key = queryKey()
     const states: UseQueryResult<string>[] = []
 
     function Page() {
       const [toggle, setToggle] = React.useState(false)
 
-      React.useEffect(() => {
-        setActTimeout(() => {
-          setToggle(true)
-        }, 20)
-      }, [setToggle])
-
-      return toggle ? <Component key="1" /> : <Component key="2" />
+      return (
+        <div>
+          <button onClick={() => setToggle(true)}>toggle</button>
+          {toggle ? (
+            <Component key="2" value="2" />
+          ) : (
+            <Component key="1" value="1" />
+          )}
+        </div>
+      )
     }
 
-    function Component() {
+    function Component({ value }: { value: string }) {
       const state = useQuery(
         key,
         async () => {
-          await sleep(5)
-          return 'data'
+          await sleep(10)
+          return 'data: ' + value
         },
         {
           cacheTime: 0,
+          notifyOnChangeProps: 'all',
         }
       )
       states.push(state)
-      return null
+      return (
+        <div>
+          <div>{state.data}</div>
+        </div>
+      )
     }
 
-    renderWithClient(queryClient, <Page />)
+    const rendered = renderWithClient(queryClient, <Page />)
 
-    await sleep(100)
+    await rendered.findByText('data: 1')
 
-    expect(states.length).toBe(5)
+    rendered.getByRole('button', { name: /toggle/i }).click()
+
+    await rendered.findByText('data: 2')
+
+    expect(states.length).toBe(4)
     // First load
-    expect(states[0]).toMatchObject({ isLoading: true, isSuccess: false })
+    expect(states[0]).toMatchObject({
+      isLoading: true,
+      isSuccess: false,
+      isFetching: true,
+    })
     // First success
-    expect(states[1]).toMatchObject({ isLoading: false, isSuccess: true })
-    // Switch
-    expect(states[2]).toMatchObject({ isLoading: false, isSuccess: true })
-    // Second load
-    expect(states[3]).toMatchObject({ isLoading: true, isSuccess: false })
+    expect(states[1]).toMatchObject({
+      isLoading: false,
+      isSuccess: true,
+      isFetching: false,
+    })
+    // Switch, goes to fetching
+    expect(states[2]).toMatchObject({
+      isLoading: false,
+      isSuccess: true,
+      isFetching: true,
+    })
     // Second success
-    expect(states[4]).toMatchObject({ isLoading: false, isSuccess: true })
+    expect(states[3]).toMatchObject({
+      isLoading: false,
+      isSuccess: true,
+      isFetching: false,
+    })
   })
 
   it('should not get into an infinite loop when removing a query with cacheTime 0 and rerendering', async () => {
@@ -5097,64 +5123,6 @@ describe('useQuery', () => {
 
       onlineMock.mockRestore()
     })
-
-    it('online queries with cacheTime:0 should not fetch if paused and then unmounted', async () => {
-      const key = queryKey()
-      let count = 0
-
-      function Component() {
-        const state = useQuery({
-          queryKey: key,
-          queryFn: async () => {
-            count++
-            await sleep(10)
-            return 'data' + count
-          },
-          cacheTime: 0,
-        })
-
-        return (
-          <div>
-            <div>
-              status: {state.status}, fetchStatus: {state.fetchStatus}
-            </div>
-            <div>data: {state.data}</div>
-          </div>
-        )
-      }
-
-      function Page() {
-        const [show, setShow] = React.useState(true)
-
-        return (
-          <div>
-            {show && <Component />}
-            <button onClick={() => setShow(false)}>hide</button>
-          </div>
-        )
-      }
-
-      const onlineMock = mockNavigatorOnLine(false)
-
-      const rendered = renderWithClient(queryClient, <Page />)
-
-      await waitFor(() =>
-        rendered.getByText('status: loading, fetchStatus: paused')
-      )
-
-      rendered.getByRole('button', { name: /hide/i }).click()
-
-      onlineMock.mockReturnValue(true)
-      window.dispatchEvent(new Event('online'))
-
-      await sleep(15)
-
-      expect(queryClient.getQueryState(key)).not.toBeDefined()
-
-      expect(count).toBe(0)
-
-      onlineMock.mockRestore()
-    })
   })
 
   describe('networkMode always', () => {

src/reactjs/useBaseQuery.ts

@@ -59,12 +59,6 @@ export function useBaseQuery<
     if (typeof defaultedOptions.staleTime !== 'number') {
       defaultedOptions.staleTime = 1000
     }
-
-    // Set cache time to 1 if the option has been set to 0
-    // when using suspense to prevent infinite loop of fetches
-    if (defaultedOptions.cacheTime === 0) {
-      defaultedOptions.cacheTime = 1
-    }
   }
 
   if (defaultedOptions.suspense || defaultedOptions.useErrorBoundary) {