Skip to content
This repository has been archived by the owner on Nov 20, 2020. It is now read-only.

Return type of the mutate function #89

Closed
leeor opened this issue Dec 26, 2019 · 6 comments
Closed

Return type of the mutate function #89

leeor opened this issue Dec 26, 2019 · 6 comments

Comments

@leeor
Copy link

leeor commented Dec 26, 2019

The mutate function, the first item in the tuple returned from useMutation() is defined to be of the type:

https://github.com/Astrocoders/reason-apollo-hooks/blob/b5d9c32ad7f7691f7d4f0d6887465815c9b7e26e/src/ApolloHooksMutation.re#L64-L73

When called, the mutation promise is handled here:

https://github.com/Astrocoders/reason-apollo-hooks/blob/b5d9c32ad7f7691f7d4f0d6887465815c9b7e26e/src/ApolloHooksMutation.re#L143-L155

Seems to me that mutation('a) should return a promise resolving to result('a) instead:

https://github.com/Astrocoders/reason-apollo-hooks/blob/b5d9c32ad7f7691f7d4f0d6887465815c9b7e26e/src/ApolloHooksMutation.re#L13-L16

The mutation('a) type was added in d4cc1a5.

@mbirkegaard
Copy link
Collaborator

mbirkegaard commented Jan 7, 2020

I agree.

We're currently handling mutation responses like this. The Loading and Called cases at the end shouldn't be there.

      mutate(~variables=input', ())
      |> Js.Promise.then_(
           (
             response:
               ReasonApolloHooks.Mutation.controlledVariantResult({
                 .
                 "response": option(string),
               }),
           ) =>
           switch (response) {
           | Data(x) =>
             switch (x##response) {
             | Some(id) => Js.Promise.resolve(Belt.Result.Ok(id))
             | None =>
               Js.Promise.resolve(Belt.Result.Error(`noClientIdInResponse))
             }
           | NoData => Js.Promise.resolve(Belt.Result.Error(`emptyResponse))
           | Error(errors) =>
             Js.Promise.resolve(Belt.Result.Error(`apolloErrors(errors)))
           /* This variant shouldn't contain Loading and Called */
           | Loading
           | Called => Js.Promise.resolve(Belt.Result.Error(`emptyResponse))
           }
         )

@fakenickels
Copy link
Member

Good catch!

@mbirkegaard
Copy link
Collaborator

mbirkegaard commented Jan 7, 2020

Good catch!

Yeah. Thanks @leeor!

@fakenickels It's a quick fix, so I can open a PR later today

@mbirkegaard
Copy link
Collaborator

@fakenickels It's a quick fix, so I can open a PR later today

"He said and did it a week later"

@fakenickels
Copy link
Member

Haha no problem!

@mbirkegaard
Copy link
Collaborator

@fakenickels I didn't use the correct keyword in the PR yesterday, so this wasn't automatically closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants