Unnecessary duplicate logging of errors to console

[Svish]

I have these hooks:

export function useGetMessages(select) {
  const company = useSelectedCompany();
  return useQuery(['messages', 'company', company?.id), { select });
}

export function useGetMessage(messageId) {
  const selector = useCallback(
    (messages) => {
      const id = Number(messageId);
      const message = messages.find((m) => m.id === id);
      if (message == null) throw new MessageNotFoundError(id);
      return message;
    },
    [messageId]
  );
  return useGetMessages(selector);
}

export class MessageNotFoundError extends Error {
  public readonly messageId: number | string;
  constructor(messageId: number | string) {
    super(`Message with id ${messageid} does not exist`);
    this.messageId = messageId;
  }
}

When I go to a page where the useGetMessage hook is used, I first of all don't really expect any errors in the console, since they should be handled by react-query and not "bubble up" anywhere? But if that's not the way I expect, I would at least expect there to be no more than one of those errors in the console?

Instead I get 6(!) log messages with that error. The error object seems to be thrown from where I expect it to (error starts with at Object.select messages.ts:76, which is the line of my throw above), but the log messages seem to come from queryObserver.js:331. If I also have the react query dev tools open, I seem to get yet another 6 error messages logged to the console from react_devtools_backend.js:3973. So that's 12(!) error messages flooding the console for every 1 actual error (from my point of view as a developer).

Is this "how it should be"? Finding it very noisy and annoying 🤔

[TkDodo]

can you show this behaviour in a codesandbox please?

it might be worth noticing that in v4, error logging will be done in development mode only, and you can also provide a different logger that doesn't log errors at all.

[Svish]

Don't know how to share a codesandbox (do I need to pay for it or something?), but pasting this code into a https://codesandbox.io with React and react-query added as dependency seems to replicate it as far as I can understand:

import { useQuery, QueryClientProvider, QueryClient } from "react-query";
import { ReactQueryDevtools } from "react-query/devtools";

const queryClient = new QueryClient();

export default function App() {
  return (
    <QueryClientProvider client={queryClient}>
      <Example />
      <ReactQueryDevtools initialIsOpen={false} />
    </QueryClientProvider>
  );
}

function Example() {
  const query = useQuery("foo", fetchFoo, {
    select: stableSelect
  });

  switch (query.status) {
    case "error":
      return <output>{query.error.message}</output>;

    case "success":
      return <output>{query.data}</output>;

    default:
      return <output>Loading...</output>;
  }
}

function stableSelect(data) {
  // Pretend something failed
  throw new Error("something failed");
}

async function fetchFoo() {
  await new Promise((resolve) => setTimeout(resolve, 1000));
  return "foo";
}

When I run this I immediately get 4 errors in the console log, and if I then open the query devtools then I get at least 4 more. Then more seem to pop up even after that, not sure why exactly. Some of it might be the default revalidation, but even then it shouldn't be that many errors being logged in my opinion.

[Svish]

Out of curiosity I also tried to throw an error inside the fetch function itself, and that error is only logged once as I would expect. And opening the developer tools results in no new errors logged at all, which also would be my expectation.

The same issue (4 errors logged) also happens if I remove the query dev tools completely.

So... there seems to be something off with how many times the select function is called? 🤔🤷‍♂️

[TkDodo]

Don't know how to share a codesandbox

you just need to log in, it's free. here is the sandbox: https://codesandbox.io/s/error-logging-peichc?file=/src/App.jsx

select gets re-run on the next render if it has an error. This was done to fix memoization issues:

• memoized selectors and errors #1855
• fix(queryObserver): re-run selector if it throws error #1875

But maybe the fix isn't good enough? I don't see why we shouldn't just cache the error and put the query in error state without re-running the selector. After all, if its stable and no dependency has changed - why would we need to re-run it? It should return the same error anyways!

If you want, you can take a look at coming up with a PR for that? My guess is it might be enough to:

• remove the !this.previousSelectError check here:
  https://github.com/tannerlinsley/react-query/blob/e97d2bc8e4b2ae8665d548166ea107ba31abeba2/src/core/queryObserver.ts#L512

this should cache the error until we get new data or a new select fn reference.

• Then, we need to put the result in error state, like we do in the catch clause here (minus the logging and the assignment to this.previousSelectError):
  https://github.com/tannerlinsley/react-query/blob/e97d2bc8e4b2ae8665d548166ea107ba31abeba2/src/core/queryObserver.ts#L526-L532
• once we get new data or a new select function reference, we will set previousSelectError to null and re-run the select function anyways :)

[TkDodo]

Huh, so I have a git diff that passes all tests and also gets rid of some duplication between select and select on placeholder data. We'd need some tests to see if it makes your situation better, but it might be a good starting point:

diff --git a/src/core/queryObserver.ts b/src/core/queryObserver.ts
index 77480e5..b540e2f 100644
--- a/src/core/queryObserver.ts
+++ b/src/core/queryObserver.ts
@@ -68,7 +68,7 @@ export class QueryObserver<
     TQueryKey
   >
   private previousQueryResult?: QueryObserverResult<TData, TError>
-  private previousSelectError: Error | null
+  private selectError: Error | null
   private previousSelect?: {
     fn: (data: TQueryData) => TData
     result: TData
@@ -93,7 +93,7 @@ export class QueryObserver<
     this.client = client
     this.options = options
     this.trackedProps = []
-    this.previousSelectError = null
+    this.selectError = null
     this.bindMethods()
     this.setOptions(options)
   }
@@ -500,8 +500,7 @@ export class QueryObserver<
       if (
         prevResult &&
         state.data === prevResultState?.data &&
-        options.select === this.previousSelect?.fn &&
-        !this.previousSelectError
+        options.select === this.previousSelect?.fn
       ) {
         data = this.previousSelect.result
       } else {
@@ -514,13 +513,10 @@ export class QueryObserver<
             fn: options.select,
             result: data,
           }
-          this.previousSelectError = null
+          this.selectError = null
         } catch (selectError) {
           getLogger().error(selectError)
-          error = selectError
-          this.previousSelectError = selectError
-          errorUpdatedAt = Date.now()
-          status = 'error'
+          this.selectError = selectError
         }
       }
     }
@@ -557,13 +553,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
           }
         }
       }
@@ -575,6 +568,12 @@ export class QueryObserver<
       }
     }
 
+    if (this.selectError) {
+      error = this.selectError as any
+      errorUpdatedAt = Date.now()
+      status = 'error'
+    }
+
     const result: QueryObserverBaseResult<TData, TError> = {
       status,
       isLoading: status === 'loading',

moreover, I think it fixes another issue where we don't have a test yet if select succeeds once, and then fails, it will give you data as undefined. However, it should use the previously stored data. I've reproduced this in this sandbox: https://codesandbox.io/s/error-logging-forked-zliyc1?file=/src/App.jsx

first fetch succeeds, then the second fetch fails (can be triggered with window refocus). data is shown as undefined, but it should show the (stale) data from the last successful fetch. That's how it works when the queryFn has an error, too - you can be in error state and still have stale data.

[TkDodo]

PR: #3554