Repository: tannerlinsley/react-query
Pull request #1737 (2021-02-03 18:08:28)

fix: notify on updates between mounting and subscribing
Theoretically query observers can miss updates from a query if there was a query update between creating the observer and subscribing to the observer. With this change the observer will check if anything changed on subscribing and update if needed. Hopefully fixes https://github.com/tannerlinsley/react-query/issues/1657 .

vercel[bot]: This pull request is being automatically deployed with Vercel ([learn more](https://vercel.link/github-learn-more)).  
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: [https://vercel.com/tannerlinsley/react-query/duiuhqtr1](https://vercel.com/tannerlinsley/react-query/duiuhqtr1)  
✅ Preview: *In Progress*



Patch:
diff --git a/src/core/infiniteQueryObserver.ts b/src/core/infiniteQueryObserver.ts
index 22458612d..959572a08 100644
--- a/src/core/infiniteQueryObserver.ts
+++ b/src/core/infiniteQueryObserver.ts
@@ -98,11 +98,9 @@ export class InfiniteQueryObserver<
     })
   }
 
-  protected getNewResult(
-    willFetch?: boolean
-  ): InfiniteQueryObserverResult<TData, TError> {
+  protected getNewResult(): InfiniteQueryObserverResult<TData, TError> {
     const { state } = this.getCurrentQuery()
-    const result = super.getNewResult(willFetch)
+    const result = super.getNewResult()
     return {
       ...result,
       fetchNextPage: this.fetchNextPage,
diff --git a/src/core/queryObserver.ts b/src/core/queryObserver.ts
index 986326aab..13938952c 100644
--- a/src/core/queryObserver.ts
+++ b/src/core/queryObserver.ts
@@ -87,6 +87,7 @@ export class QueryObserver<
         this.executeFetch()
       }
 
+      this.updateResult()
       this.updateTimers()
     }
   }
@@ -155,7 +156,6 @@ export class QueryObserver<
     options?: QueryObserverOptions<TQueryFnData, TError, TData, TQueryData>
   ): void {
     const prevOptions = this.options
-    const prevQuery = this.currentQuery
 
     this.options = this.client.defaultQueryObserverOptions(options)
 
@@ -171,23 +171,22 @@ export class QueryObserver<
       this.options.queryKey = prevOptions.queryKey
     }
 
-    this.updateQuery()
+    const didUpdateQuery = this.updateQuery()
 
-    // Take no further actions if there are no subscribers
-    if (!this.listeners.length) {
-      return
-    }
+    let optionalFetch
+    let updateStaleTimeout
+    let updateRefetchInterval
 
-    // If we subscribed to a new query, optionally fetch and update refetch
-    if (this.currentQuery !== prevQuery) {
-      this.optionalFetch()
-      this.updateTimers()
-      return
+    // If we subscribed to a new query, optionally fetch and update intervals
+    if (didUpdateQuery) {
+      optionalFetch = true
+      updateStaleTimeout = true
+      updateRefetchInterval = true
     }
 
     // Optionally fetch if the query became enabled
     if (this.options.enabled !== false && prevOptions.enabled === false) {
-      this.optionalFetch()
+      optionalFetch = true
     }
 
     // Update stale interval if needed
@@ -195,7 +194,7 @@ export class QueryObserver<
       this.options.enabled !== prevOptions.enabled ||
       this.options.staleTime !== prevOptions.staleTime
     ) {
-      this.updateStaleTimeout()
+      updateStaleTimeout = true
     }
 
     // Update refetch interval if needed
@@ -203,7 +202,29 @@ export class QueryObserver<
       this.options.enabled !== prevOptions.enabled ||
       this.options.refetchInterval !== prevOptions.refetchInterval
     ) {
-      this.updateRefetchInterval()
+      updateRefetchInterval = true
+    }
+
+    // Fetch only if there are subscribers
+    if (this.hasListeners()) {
+      if (optionalFetch) {
+        this.optionalFetch()
+      }
+    }
+
+    // Update result when subscribing to a new query
+    if (didUpdateQuery) {
+      this.updateResult()
+    }
+
+    // Update intervals only if there are subscribers
+    if (this.hasListeners()) {
+      if (updateStaleTimeout) {
+        this.updateStaleTimeout()
+      }
+      if (updateRefetchInterval) {
+        this.updateRefetchInterval()
+      }
     }
   }
 
@@ -302,12 +323,7 @@ export class QueryObserver<
 
     this.staleTimeoutId = setTimeout(() => {
       if (!this.currentResult.isStale) {
-        const prevResult = this.currentResult
         this.updateResult()
-        this.notify({
-          listeners: this.shouldNotifyListeners(prevResult, this.currentResult),
-          cache: true,
-        })
       }
     }, timeout)
   }
@@ -353,9 +369,7 @@ export class QueryObserver<
     this.refetchIntervalId = undefined
   }
 
-  protected getNewResult(
-    willFetch?: boolean
-  ): QueryObserverResult<TData, TError> {
+  protected getNewResult(): QueryObserverResult<TData, TError> {
     const { state } = this.currentQuery
     let { isFetching, status } = state
     let isPreviousData = false
@@ -364,7 +378,7 @@ export class QueryObserver<
     let dataUpdatedAt = state.dataUpdatedAt
 
     // Optimistically set status to loading if we will start fetching
-    if (willFetch) {
+    if (!this.hasListeners() && this.willFetchOnMount()) {
       isFetching = true
       if (!dataUpdatedAt) {
         status = 'loading'
@@ -442,7 +456,7 @@ export class QueryObserver<
   }
 
   private shouldNotifyListeners(
-    prevResult: QueryObserverResult,
+    prevResult: QueryObserverResult | undefined,
     result: QueryObserverResult
   ): boolean {
     const { notifyOnChangeProps, notifyOnChangePropsExclusions } = this.options
@@ -451,6 +465,10 @@ export class QueryObserver<
       return false
     }
 
+    if (!prevResult) {
+      return true
+    }
+
     if (!notifyOnChangeProps && !notifyOnChangePropsExclusions) {
       return true
     }
@@ -485,39 +503,60 @@ export class QueryObserver<
     return false
   }
 
-  private updateResult(willFetch?: boolean): void {
-    const result = this.getNewResult(willFetch)
+  private updateResult(action?: Action<TData, TError>): void {
+    const prevResult = this.currentResult as
+      | QueryObserverResult<TData, TError>
+      | undefined
+
+    const result = this.getNewResult()
 
     // Keep reference to the current state on which the current result is based on
     this.currentResultState = this.currentQuery.state
 
     // Only update if something has changed
-    if (!shallowEqualObjects(result, this.currentResult)) {
-      this.currentResult = result
+    if (shallowEqualObjects(result, prevResult)) {
+      return
+    }
 
-      if (this.options.notifyOnChangeProps === 'tracked') {
-        const addTrackedProps = (prop: keyof QueryObserverResult) => {
-          if (!this.trackedProps.includes(prop)) {
-            this.trackedProps.push(prop)
-          }
+    this.currentResult = result
+
+    if (this.options.notifyOnChangeProps === 'tracked') {
+      const addTrackedProps = (prop: keyof QueryObserverResult) => {
+        if (!this.trackedProps.includes(prop)) {
+          this.trackedProps.push(prop)
         }
-        this.trackedCurrentResult = {} as QueryObserverResult<TData, TError>
-
-        Object.keys(result).forEach(key => {
-          Object.defineProperty(this.trackedCurrentResult, key, {
-            configurable: false,
-            enumerable: true,
-            get() {
-              addTrackedProps(key as keyof QueryObserverResult)
-              return result[key as keyof QueryObserverResult]
-            },
-          })
-        })
       }
+      this.trackedCurrentResult = {} as QueryObserverResult<TData, TError>
+
+      Object.keys(result).forEach(key => {
+        Object.defineProperty(this.trackedCurrentResult, key, {
+          configurable: false,
+          enumerable: true,
+          get() {
+            addTrackedProps(key as keyof QueryObserverResult)
+            return result[key as keyof QueryObserverResult]
+          },
+        })
+      })
+    }
+
+    // Determine which callbacks to trigger
+    const notifyOptions: NotifyOptions = { cache: true }
+
+    if (action?.type === 'success') {
+      notifyOptions.onSuccess = true
+    } else if (action?.type === 'error') {
+      notifyOptions.onError = true
     }
+
+    if (this.shouldNotifyListeners(prevResult, result)) {
+      notifyOptions.listeners = true
+    }
+
+    this.notify(notifyOptions)
   }
 
-  private updateQuery(): void {
+  private updateQuery(): boolean {
     const prevQuery = this.currentQuery
 
     const query = this.client
@@ -528,7 +567,7 @@ export class QueryObserver<
       )
 
     if (query === prevQuery) {
-      return
+      return false
     }
 
     this.previousQueryResult = this.currentResult
@@ -536,54 +575,19 @@ export class QueryObserver<
     this.initialDataUpdateCount = query.state.dataUpdateCount
     this.initialErrorUpdateCount = query.state.errorUpdateCount
 
-    const willFetch = prevQuery
-      ? this.willFetchOptionally()
-      : this.willFetchOnMount()
-
-    this.updateResult(willFetch)
-
-    if (!this.hasListeners()) {
-      return
+    if (this.hasListeners()) {
+      prevQuery?.removeObserver(this)
+      this.currentQuery.addObserver(this)
     }
 
-    prevQuery?.removeObserver(this)
-    this.currentQuery.addObserver(this)
-
-    if (
-      this.shouldNotifyListeners(this.previousQueryResult, this.currentResult)
-    ) {
-      this.notify({ listeners: true })
-    }
+    return true
   }
 
   onQueryUpdate(action: Action<TData, TError>): void {
-    // Store current result and get new result
-    const prevResult = this.currentResult
-    this.updateResult()
-    const currentResult = this.currentResult
-
-    // Update timers
-    this.updateTimers()
-
-    // Do not notify if the nothing has changed
-    if (prevResult === currentResult) {
-      return
-    }
-
-    // Determine which callbacks to trigger
-    const notifyOptions: NotifyOptions = {}
-
-    if (action.type === 'success') {
-      notifyOptions.onSuccess = true
-    } else if (action.type === 'error') {
-      notifyOptions.onError = true
-    }
-
-    if (this.shouldNotifyListeners(prevResult, currentResult)) {
-      notifyOptions.listeners = true
+    this.updateResult(action)
+    if (this.hasListeners()) {
+      this.updateTimers()
     }
-
-    this.notify(notifyOptions)
   }
 
   private notify(notifyOptions: NotifyOptions): void {


Test patch:
diff --git a/src/core/tests/queryClient.test.tsx b/src/core/tests/queryClient.test.tsx
index 8b7c241a4..a754a7d0b 100644
--- a/src/core/tests/queryClient.test.tsx
+++ b/src/core/tests/queryClient.test.tsx
@@ -77,6 +77,7 @@ describe('queryClient', () => {
       const observer = new QueryObserver(queryClient, {
         queryKey: [key],
         retry: false,
+        enabled: false,
       })
       const { status } = await observer.refetch()
       expect(status).toBe('error')
diff --git a/src/core/tests/queryObserver.test.tsx b/src/core/tests/queryObserver.test.tsx
index abdd49320..a540f98ea 100644
--- a/src/core/tests/queryObserver.test.tsx
+++ b/src/core/tests/queryObserver.test.tsx
@@ -49,6 +49,24 @@ describe('queryObserver', () => {
     expect(results[2]).toMatchObject({ data: 2, status: 'success' })
   })
 
+  test('should notify when the query has updated before subscribing', async () => {
+    const key = queryKey()
+    const results: QueryObserverResult[] = []
+    const observer = new QueryObserver(queryClient, {
+      queryKey: key,
+      queryFn: () => 1,
+      staleTime: Infinity,
+    })
+    queryClient.setQueryData(key, 2)
+    const unsubscribe = observer.subscribe(result => {
+      results.push(result)
+    })
+    await sleep(1)
+    unsubscribe()
+    expect(results.length).toBe(1)
+    expect(results[0]).toMatchObject({ data: 2, status: 'success' })
+  })
+
   test('should be able to fetch with a selector', async () => {
     const key = queryKey()
     const observer = new QueryObserver(queryClient, {
diff --git a/src/react/tests/useQuery.test.tsx b/src/react/tests/useQuery.test.tsx
index 472bd6166..41ef4c869 100644
--- a/src/react/tests/useQuery.test.tsx
+++ b/src/react/tests/useQuery.test.tsx
@@ -333,6 +333,34 @@ describe('useQuery', () => {
     expect(onSuccess).toHaveBeenCalledWith('data')
   })
 
+  it('should call onSuccess after a query has been refetched', async () => {
+    const key = queryKey()
+    const states: UseQueryResult<string>[] = []
+    const onSuccess = jest.fn()
+
+    function Page() {
+      const state = useQuery(key, () => 'data', { onSuccess })
+
+      states.push(state)
+
+      const { refetch } = state
+
+      React.useEffect(() => {
+        setActTimeout(() => {
+          refetch()
+        }, 10)
+      }, [refetch])
+
+      return null
+    }
+
+    renderWithClient(queryClient, <Page />)
+
+    await sleep(50)
+    expect(states.length).toBe(4)
+    expect(onSuccess).toHaveBeenCalledTimes(2)
+  })
+
   it('should call onSuccess after a disabled query has been fetched', async () => {
     const key = queryKey()
     const states: UseQueryResult<string>[] = []
@@ -1794,11 +1822,13 @@ describe('useQuery', () => {
     unsubscribe()
 
     // 1. Subscribe observer
-    // 2. Query init
-    // 3. Query fetch
-    // 4. Query stale
-    // 5. Unsubscribe observer
-    expect(fn).toHaveBeenCalledTimes(5)
+    // 2. Query loading
+    // 3. Observer loading
+    // 4. Query success
+    // 5. Observer success
+    // 6. Query stale
+    // 7. Unsubscribe observer
+    expect(fn).toHaveBeenCalledTimes(7)
   })
 
   it('should not re-render when it should only re-render on data changes and the data did not change', async () => {
@@ -1904,6 +1934,38 @@ describe('useQuery', () => {
     expect(queryCache.find(key)!.options.queryFn).toBe(queryFn1)
   })
 
+  it('should render correct states even in case of useEffect triggering delays', async () => {
+    const key = queryKey()
+    const states: UseQueryResult<string>[] = []
+
+    const originalUseEffect = React.useEffect
+
+    // Try to simulate useEffect timing delay
+    React.useEffect = (...args: any[]) => {
+      originalUseEffect(() => {
+        setTimeout(() => {
+          args[0]()
+        }, 10)
+      }, args[1])
+    }
+
+    function Page() {
+      const state = useQuery(key, () => 'data', { staleTime: Infinity })
+      states.push(state)
+      return null
+    }
+
+    renderWithClient(queryClient, <Page />)
+    queryClient.setQueryData(key, 'data')
+    await sleep(50)
+
+    React.useEffect = originalUseEffect
+
+    expect(states.length).toBe(2)
+    expect(states[0]).toMatchObject({ status: 'loading' })
+    expect(states[1]).toMatchObject({ status: 'success' })
+  })
+
   it('should batch re-renders', async () => {
     const key = queryKey()