fix(queryObserver): cache select errors (#3554)

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.

Author: TkDodo

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',

src/core/tests/queryObserver.test.tsx

@@ -245,26 +245,25 @@ 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)
     })
     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()
   })
 

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>> = []