From d882161d09f07566f3adb3bb9f6fef56a346ce7c Mon Sep 17 00:00:00 2001
From: Dominik Dorfmeister <office@dorfmeister.cc>
Date: Sun, 1 May 2022 22:36:24 +0200
Subject: [PATCH] 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.
---
 src/core/queryObserver.ts             | 15 +++----
 src/core/tests/queryObserver.test.tsx | 61 ++++++++++++++++++++++++---
 2 files changed, 60 insertions(+), 16 deletions(-)

diff --git a/src/core/queryObserver.ts b/src/core/queryObserver.ts
index c31dc6b9ac..711ad603ac 100644
--- a/src/core/queryObserver.ts
+++ b/src/core/queryObserver.ts
@@ -462,21 +462,18 @@ 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) {
           if (process.env.NODE_ENV !== 'production') {
             this.client.getLogger().error(selectError)
@@ -521,7 +518,7 @@ export class QueryObserver<
                 placeholderData
               )
             }
-            this.previousSelectError = null
+            this.selectError = null
           } catch (selectError) {
             if (process.env.NODE_ENV !== 'production') {
               this.client.getLogger().error(selectError)
diff --git a/src/core/tests/queryObserver.test.tsx b/src/core/tests/queryObserver.test.tsx
index 2a1f9a6299..6bd4e82516 100644
--- a/src/core/tests/queryObserver.test.tsx
+++ b/src/core/tests/queryObserver.test.tsx
@@ -249,14 +249,13 @@ describe('queryObserver', () => {
   test('should always run the selector again if selector throws an error', async () => {
     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,10 +284,58 @@ 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,
     })
   })