useMutation adds undefined to TResult of MutateFunction type

[Haaxor1689]

Describe the bug

useMutation changes the return type of provided promise by adding undefined to it.

To Reproduce

const apiCall = () => new Promise<string>(resolve => resolve());

const useExample = async () => {
	const [mutateCall] = useMutation(apiCall);
	const response = await apiCall(); // string
	const mutateR = await mutateCall(); // string | undefined
};

Wrapping a function in useMutation adds a undefined to it's returned promise

Expected behavior

The provided function's return type should not be changed.

Desktop (please complete the following information):

• Version: 23

Additional context

I don't see any reason why this would be required. Together with #1077 this is making migration to this library pretty unnecessarily painful by imposing this opinionated types.

[boschni]

Hi @Haaxor1689! Issue #1077 will be fixed in V3 and this type is actually correct because the mutate function by default returns undefined when the mutation fails

[TkDodo]

In the onSuccess handler of the mutation, the result is correctly typed, so I think this is fine.

[simonedavico]

@boschni this makes using the hook really painful, because we have to deal with undefined even if we make sure to be in the success case.

[TkDodo]

@simonedavico did you read my comment? If you want to make sure that you are in the success state, I think you should use the onSuccess callback:

const apiCall = () => new Promise<string>(resolve => resolve());

const useExample = async () => {
    const [mutateCall] = useMutation(apiCall);
    await mutateCall({
        onSuccess: (mutateR) => { // mutateR will be of type string here ... }
    }); 
};

[Haaxor1689]

@TkDodo but this absolutely breaks the point of using await since you provide a callback. Also doesn't this override the onSuccess provided to useMutation where you are actually supposed to do what this hook is for, clear cache and stuff?

As I said I'm migrating an existing project to React Query and this exact enforcement seems pretty opinionated. Most common use case you can expect is wrapping a fetch in useMutation. Fetch does not return undefined if it fails and I don't see any other real example where I would like to return undefined if the promise fails.

Right now it seems much easier to just not use useMutation at all and do the cache cleanup after awaiting my fetch promise.

[TkDodo]

also doesn't this override the onSuccess provided to useMutation where you are actually supposed to do what this hook is for, clear cache and stuff?

It does not. From the docs:

onSuccess: Function(data, variables) => Promise | undefined
Optional
This function will fire when the mutation is successful and will be passed the mutation's result.
Fires after the mutate-level onSuccess handler (if it is defined)
If a promise is returned, it will be awaited and resolved before proceeding

Fetch does not return undefined if it fails and I don't see any other real example where I would like to return undefined if the promise fails.

You can enable throwOnError to make your mutation throw. Not sure if it would be safe to say that if you use throwOnError, the mutation function should return T rather than T | undefined ... 🤔 . @boschni ?

[simonedavico]

@simonedavico did you read my comment? If you want to make sure that you are in the success state, I think you should use the onSuccess callback:

const apiCall = () => new Promise<string>(resolve => resolve());

const useExample = async () => {
    const [mutateCall] = useMutation(apiCall);
    await mutateCall({
        onSuccess: (mutateR) => { // mutateR will be of type string here ... }
    }); 
};

Unfortunately not everything can be done in the onSuccess callback, for interop with other code I need from time to time to access the Promise result.
I believe that the | undefined union should be added only when throwOnError is false; if throwOnError is true, the result can never be undefined if the promise resolves.

[boschni]

Not sure if changing the types depending on an option would be possible and/or easy to do, but it also wouldn't always be correct as the option can also be set on a global level.

That said, the useMutation hook is more of a utility hook with a focus on callback usage:

const [mutate] = useMutation(apiCall);

mutate({
  onSuccess: result => {}
}); 

The reason why it does not throw by default is that otherwise users would be required to always wrap the mutate function in a try/catch. Which a lot of users would probably forget. Maybe the mutate function should not even return a promise at all to prevent any confusion?

If you do not use any of the features useMutation provides then there is not reason against using async/await.

[TkDodo]

Not sure if changing the types depending on an option would be possible and/or easy to do, but it also wouldn't always be correct as the option can also be set on a global level.

possible, probably, with a disjoint union on the boolean, but yeah, the global level makes this more difficult.

Maybe the mutate function should not even return a promise at all to prevent any confusion?

I think what it currently returns is probably unneeded, but apparently, others don't think so :)

I for one would love it if the the promises returned from onSuccess and onError would be returned from the mutate function. They are currently awaited, but the result gets lost. For example:

  const [mutatePostTodo] = useMutation(
    (text) => axios.post("/api/data", { text }),
    {
      onSuccess: (data) => {
        return Promise.resolve({ type: "success", data });
      },
      onError: (err) => {
        return Promise.resolve({ type: "error", error });
      }
    }
  );

const result = await mutatePostTodo('foo')

result could be of type:

{ type: 'error', error: unknown } | { type: 'success', data: T }

here, but I guess that's even harder because of the global query cache and onSettled ....

[simonedavico]

@boschni there is at least caching to take into account; also, just fetching in a useEffect has some pitfalls which react-query will (probably, I have not read the source code) account for.

Not all APIs are fully declarative/easy to use with the declarative approach of useMutation, unfortunately. Hiding the Promise from the caller would make impossible to wait for the call to finish.

I believe it should be possible to change the return type depending on the value of throwOnError (Material-UI's Autocomplete types do something similar), but it will probably complicate the types a lot.

[boschni]

Yeah there are a lot of opinions on the style of useMutation. I'm for example in favor of a more async/await oriented useMutation, but that would also make it more prone to user mistakes and the callback version is terser in most situations.

@simonedavico, can you explain why the callbacks wouldn't work in your case? Because I think most things can be achieved in both styles.

[Haaxor1689]

Correct me if I'm wrong but the mutate({ onSuccess, onError }) seems to me to be just promise().then().catch(). Also if you are using async/await to catch the error you actually use try catch so I see no problem in that.

So problem is that useMutation isn't really compatible with async/await.

[simonedavico]

@boschni One example that comes to mind is a form component MyForm that accepts a prop onSubmit: (values) => Promise<unknown>. MyForm uses the promise resolving/resolved/rejected state to display a loading status and disable the form inputs.

Right now this is interoperable with useMutation, if I set throwOnError: true and pass mutate to MyForm. Not sure the same would be possible with the on... callbacks though.

[TkDodo]

@simonedavico It sounds like you are trying to refactor from an existing solution to react-query, which is fine. In that case, I would think that throwOnError: true accomplishes what you want, so I see nothing wrong with that. But it looks like you are not allowing react-query to fully control the mutation, which would be more idiomatic imo:

const [mutate, isLoading] = useMutation(someFn)

<Form
    onSubmit={values => mutate(values, { onSuccess: () => ... })}
    loading={isLoading}
>
...
</Form>

Here, the useMutation handles also the loading state, and you also get access to the error object, which is nice. I don't really know what I want to be doing with onSuccess in that case 😅

If you just want useMutation to make a request, I think you can also just call the api function manually, since useMutation wouldn't really do anything then...

[simonedavico]

@TkDodo the full example is a little more complex. I am using Formik, which allows to track an isSubmitting state:

Inside MyForm, myApiCall is mutate:

    <Formik
      onSubmit={(values, { setSubmitting }) => {
        callApi(values)
          .then(() => setSubmitting(true))
          .finally(() => setSubmitting(false))
      }}
    >
      {
        ({ isSubmitting }) => (
          <form>
            { /* form fields  */ }
            <button type='submit' disabled={isSubmitting}>Save</button>     
          </form>
        )
      }
    </Formik> 

and in the parent component I can just have:

const [mutate] = useMutation(..., { throwOnError: true });

return <MyForm callApi={mutate} .... />

This allows me to rely on react-query AND keep the loading logic in the child form component. I could do it by exposing formik helpers:

<MyForm
      onSubmit={(values, { setSubmitting }) => mutate(values, {
        onSuccess: () => { setSubmitting(false) },
        onError: () => { setSubmitting(false) }
      })}
 />

But I don't like this, since I feel it breaks encapsulation. My parent component should not really know about formik helpers.

Honestly, I really do not see the advantage of the callbacks.