Repository: tannerlinsley/react-query
Pull request #3554 (2022-04-24 07:52:09)

fix: cache select errors
refs #3530

there is no need to re-run the select function if it is referentially stable, even when it throws an error. if no dependency has changed, we can re-use the cached error, as the select function should be idempotent and thus return the same error anyway.

further, if we have cached data from a previously successful select, that should be returned as `data` alongside the error.

vercel[bot]: **The latest updates on your projects**. Learn more about [Vercel for Git ↗︎](https://vercel.link/github-learn-more)

| Name | Status | Preview | Updated |
| :--- | :----- | :------ | :------ |
| **react-query** | 🔄 Building ([Inspect](https://vercel.com/tanstack/react-query/8f37LXBwU7YiBkVBRg1snzbc3afy)) |  | Apr 24, 2022 at 7:52AM (UTC) |


codesandbox[bot]: This pull request is automatically built and testable in [CodeSandbox](https://codesandbox.io).

  To see build info of the built libraries, click [here](https://ci.codesandbox.io/status/tannerlinsley/react-query/pr/3554/builds/242662) or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 4d1bcef7529cf11518f4352e2d39384dc2088cc9:

|Sandbox| Source |
|--|--|
|[tannerlinsley/react-query: basic](https://codesandbox.io/s/tannerlinsley-react-query-basic-lnhyhb)| Configuration |
|[tannerlinsley/react-query: basic-typescript](https://codesandbox.io/s/tannerlinsley-react-query-basic-typescript-g4jkkn)| Configuration |

codecov[bot]: # [Codecov](https://codecov.io/gh/tannerlinsley/react-query/pull/3554?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley) Report
> Merging [#3554](https://codecov.io/gh/tannerlinsley/react-query/pull/3554?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley) (4d1bcef) into [master](https://codecov.io/gh/tannerlinsley/react-query/commit/07c9ce01236b387d9a5c424b0a76c830a325d4b9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley) (07c9ce0) will **decrease** coverage by `0.00%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##           master    #3554      +/-   ##
==========================================
- Coverage   96.35%   96.35%   -0.01%     
==========================================
  Files          45       45              
  Lines        2278     2277       -1     
  Branches      637      637              
==========================================
- Hits         2195     2194       -1     
  Misses         80       80              
  Partials        3        3              
```


| [Impacted Files](https://codecov.io/gh/tannerlinsley/react-query/pull/3554?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley) | Coverage Δ | |
|---|---|---|
| [src/core/queryObserver.ts](https://codecov.io/gh/tannerlinsley/react-query/pull/3554/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley#diff-c3JjL2NvcmUvcXVlcnlPYnNlcnZlci50cw==) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/tannerlinsley/react-query/pull/3554?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/tannerlinsley/react-query/pull/3554?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley). Last update [07c9ce0...4d1bcef](https://codecov.io/gh/tannerlinsley/react-query/pull/3554?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tanner+Linsley).

TkDodo: working forked sandbox from the discussion with the preview build: https://codesandbox.io/s/error-logging-forked-jp52dv?file=/src/App.jsx

Patch:
diff --git a/docs/src/pages/react-native.md b/docs/src/pages/react-native.md
index ce814c4c7..a0358c062 100644
--- a/docs/src/pages/react-native.md
+++ b/docs/src/pages/react-native.md
@@ -28,11 +28,9 @@ onlineManager.setEventListener(setOnline => {
 ## Refetch on App focus
 
 In React Native you have to use React Query `focusManager` to refetch when the App is focused.
-You can use 'react-native-appstate-hook' to be notified when the App state has changed.
 
 ```ts
 import { focusManager } from 'react-query'
-import useAppState from 'react-native-appstate-hook'
 
 function onAppStateChange(status: AppStateStatus) {
   if (Platform.OS !== 'web') {
@@ -40,9 +38,11 @@ function onAppStateChange(status: AppStateStatus) {
   }
 }
 
-useAppState({
-  onChange: onAppStateChange,
-})
+useEffect(() => {
+  const subscription = AppState.addEventListener('change', onAppStateChange)
+
+  return () => subscription.remove()
+}, [])
 ```
 
 ## Refresh on Screen focus
diff --git a/src/core/queryObserver.ts b/src/core/queryObserver.ts
index c3fa0f683..7dff4fb57 100644
--- a/src/core/queryObserver.ts
+++ b/src/core/queryObserver.ts
@@ -68,11 +68,9 @@ export class QueryObserver<
     TQueryKey
   >
   private previousQueryResult?: QueryObserverResult<TData, TError>
-  private previousSelectError: Error | null
-  private previousSelect?: {
-    fn: (data: TQueryData) => TData
-    result: TData
-  }
+  private selectError: Error | null
+  private selectFn?: (data: TQueryData) => TData
+  private selectResult?: TData
   private staleTimeoutId?: number
   private refetchIntervalId?: number
   private currentRefetchInterval?: number | false
@@ -93,7 +91,7 @@ export class QueryObserver<
     this.client = client
     this.options = options
     this.trackedProps = []
-    this.previousSelectError = null
+    this.selectError = null
     this.bindMethods()
     this.setOptions(options)
   }
@@ -508,27 +506,21 @@ export class QueryObserver<
       if (
         prevResult &&
         state.data === prevResultState?.data &&
-        options.select === this.previousSelect?.fn &&
-        !this.previousSelectError
+        options.select === this.selectFn
       ) {
-        data = this.previousSelect.result
+        data = this.selectResult
       } else {
         try {
+          this.selectFn = options.select
           data = options.select(state.data)
           if (options.structuralSharing !== false) {
             data = replaceEqualDeep(prevResult?.data, data)
           }
-          this.previousSelect = {
-            fn: options.select,
-            result: data,
-          }
-          this.previousSelectError = null
+          this.selectResult = data
+          this.selectError = null
         } catch (selectError) {
           getLogger().error(selectError)
-          error = selectError
-          this.previousSelectError = selectError
-          errorUpdatedAt = Date.now()
-          status = 'error'
+          this.selectError = selectError
         }
       }
     }
@@ -565,13 +557,10 @@ export class QueryObserver<
                 placeholderData
               )
             }
-            this.previousSelectError = null
+            this.selectError = null
           } catch (selectError) {
             getLogger().error(selectError)
-            error = selectError
-            this.previousSelectError = selectError
-            errorUpdatedAt = Date.now()
-            status = 'error'
+            this.selectError = selectError
           }
         }
       }
@@ -583,6 +572,13 @@ export class QueryObserver<
       }
     }
 
+    if (this.selectError) {
+      error = this.selectError as any
+      data = this.selectResult
+      errorUpdatedAt = Date.now()
+      status = 'error'
+    }
+
     const result: QueryObserverBaseResult<TData, TError> = {
       status,
       isLoading: status === 'loading',


Test patch:
diff --git a/src/core/tests/queryObserver.test.tsx b/src/core/tests/queryObserver.test.tsx
index 1cba155cb..9ad4123a5 100644
--- a/src/core/tests/queryObserver.test.tsx
+++ b/src/core/tests/queryObserver.test.tsx
@@ -245,18 +245,17 @@ describe('queryObserver', () => {
     expect(observerResult2.data).toMatchObject({ myCount: 1 })
   })
 
-  test('should always run the selector again if selector throws an error', async () => {
+  test('should always run the selector again if selector throws an error and selector is not referentially stable', async () => {
     const consoleMock = mockConsoleError()
     const key = queryKey()
     const results: QueryObserverResult[] = []
-    const select = () => {
-      throw new Error('selector error')
-    }
     const queryFn = () => ({ count: 1 })
     const observer = new QueryObserver(queryClient, {
       queryKey: key,
       queryFn,
-      select,
+      select: () => {
+        throw new Error('selector error')
+      },
     })
     const unsubscribe = observer.subscribe(result => {
       results.push(result)
@@ -264,7 +263,7 @@ describe('queryObserver', () => {
     await sleep(1)
     await observer.refetch()
     unsubscribe()
-    expect(results.length).toBe(5)
+    expect(results.length).toBe(4)
     expect(results[0]).toMatchObject({
       status: 'loading',
       isFetching: true,
@@ -285,11 +284,60 @@ describe('queryObserver', () => {
       isFetching: false,
       data: undefined,
     })
-    expect(results[4]).toMatchObject({
+    consoleMock.mockRestore()
+  })
+
+  test('should return stale data if selector throws an error', async () => {
+    const consoleMock = mockConsoleError()
+    const key = queryKey()
+    const results: QueryObserverResult[] = []
+    let shouldError = false
+    const error = new Error('select error')
+    const observer = new QueryObserver(queryClient, {
+      queryKey: key,
+      queryFn: () => (shouldError ? 2 : 1),
+      select: num => {
+        if (shouldError) {
+          throw error
+        }
+        shouldError = true
+        return String(num)
+      },
+    })
+
+    const unsubscribe = observer.subscribe(result => {
+      results.push(result)
+    })
+    await sleep(10)
+    await observer.refetch()
+    unsubscribe()
+
+    expect(results.length).toBe(4)
+    expect(results[0]).toMatchObject({
+      status: 'loading',
+      isFetching: true,
+      data: undefined,
+      error: null,
+    })
+    expect(results[1]).toMatchObject({
+      status: 'success',
+      isFetching: false,
+      data: '1',
+      error: null,
+    })
+    expect(results[2]).toMatchObject({
+      status: 'success',
+      isFetching: true,
+      data: '1',
+      error: null,
+    })
+    expect(results[3]).toMatchObject({
       status: 'error',
       isFetching: false,
-      data: undefined,
+      data: '1',
+      error,
     })
+
     consoleMock.mockRestore()
   })
 
diff --git a/src/react/tests/useQuery.test.tsx b/src/react/tests/useQuery.test.tsx
index 8ffc82a6b..f9eb77766 100644
--- a/src/react/tests/useQuery.test.tsx
+++ b/src/react/tests/useQuery.test.tsx
@@ -820,6 +820,47 @@ describe('useQuery', () => {
     consoleMock.mockRestore()
   })
 
+  it('should not re-run a stable select when it re-renders if selector throws an error', async () => {
+    const consoleMock = mockConsoleError()
+    const key = queryKey()
+    const error = new Error('Select Error')
+    let runs = 0
+
+    function Page() {
+      const [, rerender] = React.useReducer(() => ({}), {})
+      const state = useQuery<string, Error>(
+        key,
+        () => (runs === 0 ? 'test' : 'test2'),
+        {
+          select: React.useCallback(() => {
+            runs++
+            throw error
+          }, []),
+        }
+      )
+      return (
+        <div>
+          <div>error: {state.error?.message}</div>
+          <button onClick={rerender}>rerender</button>
+          <button onClick={() => state.refetch()}>refetch</button>
+        </div>
+      )
+    }
+
+    const rendered = renderWithClient(queryClient, <Page />)
+
+    await waitFor(() => rendered.getByText('error: Select Error'))
+    expect(runs).toEqual(1)
+    fireEvent.click(rendered.getByRole('button', { name: 'rerender' }))
+    await sleep(10)
+    expect(runs).toEqual(1)
+    fireEvent.click(rendered.getByRole('button', { name: 'refetch' }))
+    await sleep(10)
+    expect(runs).toEqual(2)
+
+    consoleMock.mockRestore()
+  })
+
   it('should re-render when dataUpdatedAt changes but data remains the same', async () => {
     const key = queryKey()
     const states: UseQueryResult<string>[] = []
@@ -4134,7 +4175,7 @@ describe('useQuery', () => {
     await waitFor(() => rendered.getByText('Data: selected 3'))
   })
 
-  it('select should structually share data', async () => {
+  it('select should structurally share data', async () => {
     const key1 = queryKey()
     const states: Array<Array<number>> = []