reasonml-community / reason-apollo-hooks

Deprecated in favor of https://github.com/reasonml-community/graphql-ppx
https://reasonml-community.github.io/reason-apollo-hooks/
MIT License
137 stars 28 forks source link

Return type of the `mutate` function #89

Closed leeor closed 4 years ago

leeor commented 4 years ago

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 https://github.com/Astrocoders/reason-apollo-hooks/pull/39/commits/d4cc1a5b270cf7271f37431642b16a2e7270e2af.

mbirkegaard commented 4 years ago

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 commented 4 years ago

Good catch!

mbirkegaard commented 4 years ago

Good catch!

Yeah. Thanks @leeor!

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

mbirkegaard commented 4 years ago

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

"He said and did it a week later"

fakenickels commented 4 years ago

Haha no problem!

mbirkegaard commented 4 years ago

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