Why Doesn't `mutate` return a value on Error or Promise Rejections?
martinmckenna opened this issue · 5 comments
Hi sorry for all the GitHub issues, I'm having another issue that i'm finding I may need to create a workaround for
I have code the roughly looks like this:
export const $createThing = createMutatorStore<CreateStreamPayload>(({ data, invalidate }) => {
invalidate('things')
return createThing(data);
})
...
const { mutate } = useStore($createThing)
const handleAddNewThing = async () => {
try {
const response = await mutate({ hello: 'world' });
console.log(response) // only exist if non promise rejection
} catch (e) {
// never gets here
showToastNotification('something went wrong!')
}
}
<button onClick={handleAddNewThing} />
Now the issue here is that the only real way to show my toast notification is listen to error
and do it in a useEffect
like so:
export const $createThing = createMutatorStore<CreateStreamPayload>(({ data, invalidate }) => {
invalidate('things')
return createThing(data);
})
...
const { mutate, error } = useStore($createThing)
const handleAddNewThing = async () => {
const response = await mutate({ hello: 'world' });
console.log(response)
}
useEffect(() => showToastNotification('something went wrong!'), [error])
...
<button onClick={handleAddNewThing} />
So my question is, why can't I just get access to the error at the time I call mutate
? Even react themselves say this is not a good way of reacting to changes:
https://react.dev/learn/you-might-not-need-an-effect#sharing-logic-between-event-handlers
Essentially what would be awesome is an option that will return the error inside this catch
block:
https://github.com/nanostores/query/blob/main/lib/main.ts#L365
Hi, thanks for the awesome lib! I'm working on the same team as Martin.
I would expect there are a few reasonable choices for this.
- Bubble a thrown exception
- Return a value that wraps the exception
- Allow us to pass an error callback.
Ideally, we get to specify something like that in the createMutatorStore init.
What do you think?
Will this work for you? Seems simple enough.
const { mutate } = useStore($createThing)
const handleAddNewThing = async () => {
const response = await mutate({ hello: 'world' });
if ($createThing.value.error) { /* show toast */ }
}
Since you don't need reactivity in the callback, and $createThing
is a module variable, I don't think it can backfire in any way.
See the thing is that mutate
's expected not to throw, ever, by design. It's too simple to forget adding error handling somewhere and suddenly have shitty UX.
I also very much dislike when things are overly complicated or have too many options. As an example of behavior I don't like: Apollo Client. If you destructure error
from useMutation
OR provide onError
handler, mutate
will never throw; if you do neither, it will throw. Although I understand the motivation, it seems too smart for my taste, I'd like the fetching lib to be dumb and simple.
On a broader topic, I'd suggest moving toast state to nanostores as well. If you have everything in one state manager, you'll see how simple it is to solve such issues. In this particular case I'd most probably introduce a global onError
handler that just shows a toast for all mutations for a given context. Or, if global handler doesn't work for you for some reason, I'd at least move error handling from component and into the mutator itself (after all, mutator can theoretically be used in many places, therefore it's a good place to hold the error handling).
That makes sense. I like this lib for its simplicity, I can read the impl in one sitting. I agree implicit throw vs not throw is a debugging headache.
I've seen libs just take that behavior as a parameter, whether to reraise, wrap the error, or do something else with it. I really loved using Async Monitors: https://ocaml.org/p/async_kernel/latest/doc/Async_kernel/Monitor/index.html#val-try_with
We'll try out what you recommend, though.
hey @dkzlv this seems to work, but it seems like the MutatorStore
isn't properly typed and doesn't think value
exists on it
We'll go with this for now, but I think even having an onError
option on the actual createMutatorStore
call, much like how the option exists today as an option to nanoquery()
, would be amazing
React query also exposes an option like this:
https://tanstack.com/query/latest/docs/framework/react/guides/mutations#mutation-side-effects
@martinmckenna Oh, both MutatorStore
and FetcherStore
merely extend the MapStore
from nanostores
, and it does have value
property. It was always there in the runtime, but we only decided to make it a part of public interface a few versions back. Try updating the nanostores
lib to latest version, it should be ok.
As for your suggestion, I'll make another issue for it, and will close this one for now. Feel free to ping if something doesn't work for you!