throw in onError fails silently and stops flow, while Promise.reject() works as expected and shows Uncaught Error in console

[swingthrough]

Describe the bug

Issue

Throwing from inside onError fails silently (no hint in the console about Uncaught Error).
However, using Promise.reject works as expected and the error can be seen in the console - "Uncaught error...".
This is the case for all three onError definitions:

1. mutationCache
2. useMutation
3. mutate

And the second thing - throwing from inside onError defined in mutationCache also prevents onError defined in useMutation from running at all.
However, onError defined on mutate is called always:

• so no matter if we rejected or thrown from mutationCache
• no matter if we rejected or thrown from useMutation
onError in mutate will be called anyway.

Here's a codesandbox demo.

Your minimal, reproducible example

https://codesandbox.io/p/sandbox/sweet-villani-clpw7d?file=%2Fsrc%2FApp.tsx%3A52%2C8

Steps to reproduce

Code to reproduce the issue:

queryClient:

export const queryClient = new QueryClient({
  mutationCache: new MutationCache({
    onError: (error) => {
      console.log("[mutationCache][onError] triggered with error: ", error);

      try {
        // do something that might throw an error
        throw new Error("(ERROR A: thrown from mutationCache onError)");
      } catch (e) {
        console.log("[mutationCache][onError] caught e: ", e);
        /**
         * Throwing here (or simply not wrapping previous call in try/catch) causes it
         * to stop here - so useMutation::onError in App.tsx is not even called
         * and there is no hint in console apart from the explicit console.log(s) above
         */
        throw e;
        /**
         * Instead of throwing, rejecting like this makes useMutation::onError in App.tsx run as expected,
         * and after that there is clear "Uncaught (in promise) Error: (ERROR A: thrown from mutationCache onError)"
         * shown in the console
         */
        // Promise.reject(e);
      }
    },
  }),
});

useMutation:

const { mutate } = useMutation({
    mutationFn: async () => {
      throw new Error("DUMMY POST REQUEST FAILED");
    },
    onError: (error) => {
      /**
       * If in index.tsx in MutationCache::onError we throw, we don't even get here,
       * and there is no hint about uncaught error, only the explicit console.log(s) that are there.
       *
       * But if we use Promise.reject there, we do get here, so let's say we've done that
       * and what comes next is the following:
       */

      console.log("[useMutation][onError] triggered with error: ", error);
      try {
        // do something that might throw an error
        throw new Error("(ERROR B: thrown from useMutation onError)");
      } catch (e) {
        console.log("[useMutation][onError] caught e: ", e);
        /**
         * Throwing here (or simply not wrapping previous call in try/catch) means
         * that the only hint about ERROR B being thrown is the explicit console.log above.
         * Then after those, the only thing we will see is:
         * "Uncaught (in promise) Error: (ERROR A: thrown from mutationCache onError)"
         * which comes from the mutationCache::onError in index.tsx
         */
        throw e;
        /**
         * However, instead of throwing, if we reject, in the console we will see:
         * "Uncaught (in promise) Error: (ERROR A: thrown from mutationCache onError)"
         * followed by:
         * "Uncaught (in promise) Error: (ERROR B: thrown from useMutation onError)"
         */
        // Promise.reject(e);
      }
    },
  });

mutate:

mutate(undefined, {
  onError: (error) => {
    /**
     * Now for this onError inside mutate call, this one is called no matter
     * if we throw or reject inside the mutationCache::onError.
     * So if we reject in mutationCache, the callbacks are called in the following order:
     *   1. mutationCache::onError
     *   2. useMutation::onError
     *   3. mutate::onError
     *
     * But if we throw in mutationCache, it's like this:
     *   1. mutationCache::onError
     *   2. mutate::onError
     * So useMutation::onError is not called, but this one still is
     */
    console.log("[mutate][onError] triggered with error: ", error);
    try {
      // do something that might throw an error
      throw new Error("(ERROR C: thrown from useMutation onError)");
    } catch (e) {
      console.log("[mutate][onError] caught e: ", e);
  
      /**
       * But here it is the same as for the other cases,
       * if we throw (or don't wrap previous call in try/catch), we won't see a hint about ERROR C apart from the console.log above
       */
      throw e;
  
      /**
       * If we reject, we can see in the console:
       * "Uncaught (in promise) Error: (ERROR C: thrown from useMutation onError)"
       */
      // Promise.reject(e);
    }
  },
  });

Expected behavior

I'd expect to see the Uncaught error in the console even if I use throw.
The case for me where I discovered this issue was that I was calling Object.keys(someObject.errors) inside onError in mutationCache and using a toast to show the error.
I also had some logic in onError defined in useMutation, setting an error for a form.
In one case the errors property was undefined, which caused Object.keys to throw before the toast could show anything - onError in useMutation was not called so no form error was set. So there was no hint at all about an error occurring.

So the only safe approach here seems to be wrapping the logic in onError in mutationCache in try/catch and Promise.reject the error from the catch clause - but this seems like an extra step that should not be necessary.
And having no hint about an error occurring seems like a bug.

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

• OS: macOS
• Browser: Chrome

Tanstack Query adapter

react-query

TanStack Query version

v5.12.2

TypeScript version

No response

Additional context

No response

[TkDodo]

And the second thing - throwing from inside onError defined in mutationCache also prevents onError defined in useMutation from running at all.
However, onError defined on mutate is called always:

yeah we don't really do error handling in the error handler. We're basically inside a catch block, and another error happens inside the catch block. onSettled also won't be called, this is the code:

query/packages/query-core/src/mutation.ts

Lines 236 to 271 in 13b17ee

	} catch (error) {
	try {
	// Notify cache callback
	await this.#mutationCache.config.onError?.(
	error as any,
	variables,
	this.state.context,
	this as Mutation<unknown, unknown, unknown, unknown>,
	)
	await this.options.onError?.(
	error as TError,
	variables,
	this.state.context,
	)
	// Notify cache callback
	await this.#mutationCache.config.onSettled?.(
	undefined,
	error as any,
	this.state.variables,
	this.state.context,
	this as Mutation<unknown, unknown, unknown, unknown>,
	)
	await this.options.onSettled?.(
	undefined,
	error as TError,
	variables,
	this.state.context,
	)
	throw error
	} finally {
	this.#dispatch({ type: 'error', error: error as TError })
	}
	}

the mutationCache callback is called first, so we would need to wrap each call of onError into their own try/catch. Also, with which error should we continue to call the next onError then? The one from the actual mutation or the one from the previous onError callback? This all gets very confusing and very complicated pretty fast.

Throwing from inside onError fails silently (no hint in the console about Uncaught Error).
However, using Promise.reject works as expected and the error can be seen in the console - "Uncaught error...".

I don't see how this can be the case. execute (which executes the mutation) is an async function, and inside async functions, thrown errors are transformed into rejected Promises, so there shouldn't be a difference between throwing and rejecting. I think it's because you do:

// Promise.reject(e);

when in fact you want:

return Promise.reject(e);

If I return the rejected Promise, I see the same behaviour as with throwing

However, onError defined on mutate is called always:

Yes, that's because it's a totally different layer of error handling. It's on the MutationObserver, and it is also conditional to the component that triggered the mutation to still be mounted when the mutation finishes.

[swingthrough]

If I return the rejected Promise, I see the same behaviour as with throwing

Yeah, with the returned Promise it works the same as throwing.
So if I don't return it, it basically resolves and then later rejects, but it's already out of the error handling mechanisms and that's why it can be seen as uncaught? Or something else is happening here?

the mutationCache callback is called first, so we would need to wrap each call of onError into their own try/catch. Also, with which error should we continue to call the next onError then? The one from the actual mutation or the one from the previous onError callback? This all gets very confusing and very complicated pretty fast.

So how would you recommend structuring this use case:

1. I want mutation cache to have general error handling - basically it does some parsing and of the error and then it is shown in a toast. This happens for all mutations.
2. Let's say I have a form, e.g. login, and a useLoginMutation that has onError which on 401 sets a form error so that the user knows the credentials are invalid.

The thing is, if by any chance an error is thrown due to some parsing error in the mutation cache onError before it could show the toast, the user has no hint about something going wrong - no toast shown, and since onError in useLoginMutation is not called, no form error is set either.

I get it that the parsing error that happened is basically a bug that can be fixed. But if the error didn't fail silently, I could at least use an error boundary that would get triggered and explicitly show that there is an error in the app. It's much more obvious that there is an issue to be fixed than if it just failed silently, didn't login, didn't toast, didn't show an error.

So is there some idiomatic way of doing it?
Maybe moving setError for the form into onError for mutate would suffice since it's always called (at least if the login page is mounted)?

And also if I use Promise.reject(e) without the return as I mentioned, the error is uncaught and I could handle it in an error boundary I suppose - is there any issue with not returning the Promise?

[TkDodo]

So if I don't return it, it basically resolves and then later rejects, but it's already out of the error handling mechanisms and that's why it can be seen as uncaught? Or something else is happening here?

The promise rejects but isn't handled anywhere so it bubbles up to the browser. We can't catch it if you don't return it ;)

But if the error didn't fail silently, I could at least use an error boundary

not really because error boundaries don't catch errors that happen in side effects & event handlers.

Maybe moving setError for the form into onError for mutate would suffice since it's always called

That's a good idea anyhow because it will only be invoked when the component is still mounted. No need to set a form state if the form isn't there once the mutation resolves.

1. If you are worried about errors in your error handling please wrap the global error handler in a try/catch yourself.

[swingthrough]

not really because error boundaries don't catch errors that happen in side effects & event handlers.

Dindn't realize this. Thanks.

I have one more thing where I'm not sure how I would go about refactoring it - I've seen the global onError used like this in a project:

mutationCache: new MutationCache({
  onError: (error, vars, context, mutation) => {
    if (mutation.options.onError === undefined) {
      showToast(error)
    }
  },
}),

So if the useMutation has onError defined, the toast is not shown. Now let's say I find out that it would be better to move the logic that is in useMutation's onError to mutate, as mentioned previously (sets error to a form). Is there a way to do still do this check somehow?

[TkDodo]

Is there a way to do still do this check somehow?

Not really because a mutation could have multiple observers. We don't generally have a concept of chaining error handlers. Like, if it was handled globally, it won't be handled anymore in the component ...