useMutation types: variables are optional

[TkDodo]

Describe the bug

in TypeScript, the mutation function created by the useMutation hook can always be called without passing arguments, because the type TVariables is set to optional:

https://github.com/tannerlinsley/react-query/blob/00b9e968a80ca7c51fd50479aee852bb41bf9bc6/src/react/useMutation.ts#L132

To Reproduce

I made a codeSandbox example: https://codesandbox.io/s/async-currying-wepni

Please look at the comment at line 54

I should be forced to call that function like this:

mutatePostTodo(text)

but I can also call it like this:

mutatePostTodo()

Expected behavior

The type of variables that you need to pass to that function should be equal to the type that the actual mutation function accepts, in other words, it should be variables: TVariables rather than variables?: TVariables

Desktop (please complete the following information):

• OS: MacOS Catalina
• Browser: Chrome
• Version: 85

Additional context

All the usages of variables in that function are accessed with the bang operator, for example:

https://github.com/tannerlinsley/react-query/blob/93db5b61246dcd381d699054de48fca685bef73b/src/react/useMutation.ts#L146

or https://github.com/tannerlinsley/react-query/blob/00b9e968a80ca7c51fd50479aee852bb41bf9bc6/src/react/useMutation.ts#L174

So I don't really see why it should be optional to begin with 🤷 .

Happy to make a PR if you approve of this :)

[tannerlinsley]

Yeah a PR would be great. Be sure to include some tests too? :)

[TkDodo]

Do you have tests of the type definitions in mind, e.g. with https://github.com/Microsoft/dtslint ?

[boschni]

Think we'll just need to set it to required in V3. Doing so will prevent possible bugs, at the expense of having to call the function as mutate(undefined) when the mutation does not have any variables.

[TkDodo]

@boschni right, i didn’t think about mutations with no variables... I’ve never done that, but it’s still a legit case.

I think this TS issue is on topic, but not sure if any of the provided workarounds to make a parameter optional if its undefined would help us here: microsoft/TypeScript#12400

I agree that that would be a breaking change and we would thus need to wait for v3

[TkDodo]

I just found this discussion on the topic: #991 (comment)

Sorry for the duplicate. @tannerlinsley mentioned that type changes are not considered as “breaking”, so we could also just go ahead with making variables mandatory in v2?

[tannerlinsley]

I would be okay implementing this in v2 as non-breaking, but I'll leave that up to @boschni since he knows more about the type implications. Otherwise, expect it in v3.

[boschni]

The typing issue has been fixed in V3

[qoalu]

I am having an issue with the required parameter case.
I have a mutation function that has parameters which are all optional and there are defaults.

When I have no parameters typescript does not complain, as well as when I pass the parameters, however calling the function without parameters now causes an error

useMutation((param1?: string, param2?:string) => {})

[TkDodo]

as you can see in the signature you posted, useMutation only takes one variables argument. If you want to pass multiple arguments, you have to use an object:

useMutation((variables: { param1?: string, param2?:string} | undefined) => ...)

[qoalu]

The above function also results in a typescript error when calling it without parameters

the variables parameter is required in the signature

Tried with an object as well:

useMutation(
     ({ param1, param2 }: { param1?: string, param2?: string } = {}) => {...}, 
     ... 
)

the call I am trying to do is without params

const { mutateAsync, isLoading } = useMutation();

mutateAsync()

[TkDodo]

hmm this works fine for me, as variables are inferred to void:

      const { mutateAsync } = useMutation( () => Promise.resolve())
      mutateAsync()

if you have an optional parameter, you need to pass undefined at least I'm afraid, because that doesn't work:

      const { mutateAsync } = useMutation((foo?: string) =>
        Promise.resolve()
      )
      mutateAsync() //error here

but that does:

      const { mutateAsync } = useMutation((foo?: string) =>
        Promise.resolve()
      )
      mutateAsync(undefined)

[qoalu]

i see, I am passing an empty object as a workaround, but it does not feel elegant :)
thanks for the guidance here!