query: Thoughts on mutate function not handling rejected promises
Hey Tanner! Loving this library so far. Just spend the last little but with a snag on the fact that mutate propagates rejected promises. No big deal because I can handle those errors myself, however I don’t see any reason for react-query to not just handle them for us. For example
import React from 'react'
import ReactDOM from 'react-dom'
import {useMutation} from 'react-query'
function failingMutation() {
return new Promise((resolve, reject) => {
setTimeout(() => {
reject(new Error('oh no'))
}, 1000)
})
}
function App() {
const [mutate, {error}] = useMutation(failingMutation)
async function handleClick() {
try {
await mutate()
} catch (e) {
// ignore this because useMutation gives me the
// error that I need to display anyway.
}
}
return (
<div>
<div>{error ? error.message : 'no error yet'}</div>
<button onClick={handleClick}>Click me</button>
</div>
)
}
ReactDOM.render(<App />, document.getElementById('root'))
The point I’m trying to make here is that there’s no value in useMutation not handling the rejected promise for us because it gives us the error anyway.
As I was developing this issue, I thought that maybe this could be useful for having ErrorBoundaries handle those errors, but then I realized that because the error is happening outside of React’s callstack, a failed promise wouldn’t get handled by error boundaries anyway. So react-query could make that happen and that might be useful, but I would argue that it would be better to not, and allow users to just do this instead:
React.useEffect(() => {
if (error) throw error
}, [error])
Or maybe this could be something as an opt-in option for useMutation.
Either way, I think mutate needs to not propagate rejected promises. Otherwise we have a bit of unnecessary boilerplate to make sure we don’t have unhandled rejections.
What do you think?
About this issue
- Original URL
- State: closed
- Created 4 years ago
- Reactions: 19
- Comments: 22 (5 by maintainers)
I came to this issue because I was following the docs, and my try/catch wasn’t catching even though my axios query was definitely failing with a 400 error.
Docs should be updated from
useMutation(pingMutation)touseMutation(pingMutation, { throwOnError: true })or the example should be changed to not use try/catch. Happy to submit pull request if you let me know which way you’d like to go.Released in v0.4.0! Check out the changelog, docs, or this commit for more information: https://github.com/tannerlinsley/react-query/commit/c4bcdac16b0daf19994e055ddd562e89523d3d6e
@coodoo please provide a codesandbox reproduction.
catchRejectionsis not something I am familiar with, which means it doesn’t exist for at least 2 years 😅.In v3, the mutation functions were split into
mutateandmutateAsync:mutatereturns nothing and catches errorsmutateAsyncreturns a Promise, so you need to catch errors manually.You can read about the differences here: https://tkdodo.eu/blog/mastering-mutations-in-react-query#mutate-or-mutateasync
Hmm I think there are use cases for getting a promise still like in-scope side effects for error handling where you called the mutation. However I would be 100% in on adding an option and global default option that would make these throw or not. That would also make it a backwards compatible change too until we decide to change the default to not throw in the next major version. Thoughts? On Jan 31, 2020, 5:13 PM -0700, Kent C. Dodds notifications@github.com, wrote:
Thanks @TkDodo for the quick reply,
mutateAsyncis exactly the thing I need to manually catch the error instead of letting it showing up in the devtools console! 🙌I’m in favor of that 👍
Oh wow! Thank you so much @TkDodo for answering and totally helping me out. You are a total gem!
Maybe it was just me being easily confused, but I feel like adding some of that information to the error-handling page of docs would be great. It seems unlikely to accidentally stumble upon setLogger api page, at least to me.
Again thanks a bunch 😃
That is because react-query logs to the console per default, which you can override: https://react-query.tanstack.com/reference/setLogger
this looks pretty outdated. Only .mutateAsync needs try/catch, the example should work as-is