Passing variables only when calling executeMutation

[gugahoa]

In the PR #69 by @Schmavery where typed use mutation was introduced, there was also a suggestion on making the interface for useMutation take makeWithVariables as parameter to let users pass variables later. In hindsight, it seems this is the expected behavior by users as can be seen here: https://spectrum.chat/urql/reason/usemutation-api~72b6306a-8d92-4d17-9fff-15826dd64070 and here #103

What are the thoughts on creating another useMutation/useQuery function where it's possible to pass the variables when calling executeMutation/executeQuery?

cc @parkerziegler

[parkerziegler]

Alright, so diving back into this, I feel like the two implementations I'm seeing discussed fall into two categories.

1. Use different parts of the graphql_ppx / graphql_ppx_re API to achieve what we want. This is the implementation Avery mentioned in Typed useMutation #69, which I think is a cool idea. Rather than passing .make() with variables applied, we pass the makeWithVariables function itself.

let (result, executeMutation) = Hooks.useMutation(~request=MyQuery.makeWithVariables, ());

In this case executeMutation would essentially use makeWithVariables under the hood. What seems a bit difficult with this implementation is that I don't yet see a way to grab MyQuery.t off makeWithVariables without actually applying it, which means typing result would be difficult. This would also mean that users become responsible for parsing the returned result. Right now in useMutation we get access to that parse function by applying make, and we use that to parse the response for the user before handing it back to them.

2. We go for the functor approach similar to reason-apollo-hooks. @gugahoa scaffolded out this API a bit on Spectrum:

module type QuerySig {
  type t;
  let parse: Js.Json.t => t;
  let query: string;
};

let useMutation:
  (~request: (module QuerySig with type t = 'response)) =>
  (
    hookResponse('response),
    Js.Json.t => Js.Promise.t(UrqlClient.ClientTypes.operationResult)
  )

With this approach I think we get both the type inference and the ability to parse the response for the user (the best of both worlds), but we do lose the consistency of the APIs with useQuery and useSubscription. I can handle disparate APIs if it means we provide end users with better type inference and properly parsed responses from their backend. So I think my current preference is probably for the latter unless folks can figure out a way to have number 1 hit both correctly infer types and return a parsed result.

cc/ @robinweser @Schmavery @gugahoa any thoughts here?

[parkerziegler]

FWIW here's a proof of concept for the functor approach (the second one above) that appears to be working well in one of our examples: https://github.com/FormidableLabs/reason-urql/compare/task/functor-useMutation-api

[robinweser]

As much as I dislike the fact that it's going to break API consistency, I feel like the second approach is the better one. The first one, while elegant, feels a bit inconvenient to use... I have a use case I have to write my own parse instead of leveraging graphql-ppx due to type inconsistency in our backend. (It's a pity, but fine in that case)

[Schmavery]

Still thinking about it a little, but I can't understand the problem @parkerziegler describes here:

I don't yet see a way to grab MyQuery.t off makeWithVariables without actually applying it, which means typing result would be difficult.

wouldn't the type of useMutation be something like

let useMutation: 
  ~request=(('variables => UrqlTypes.request('response))
            => ('response, 'variables => unit))

It seems like adding an additional useDynamicMutation or something API could help the API consistency problem while allowing people to do this too.. just an idea.

@robinweser Re: custom parse function -- FWIW I included an example for this in the original change in case it's helpful, didn't want to have a hard requirement on the ppx https://github.com/FormidableLabs/reason-urql/blob/fc01954e9c8e8353f6e6248932f0fc5b995fe115/examples/3-mutation/src/Dog.re#L68-L75

[parkerziegler]

@Schmavery the challenge I was seeing is that, in order to get access to the type of the query / mutation / subscription, I think you have to call .make() on the graphql_ppx_re module. Right now the user has to do this so we can pull .parse off of what they pass us in request in order to properly infer the return type.

What @robinweser seems to be running into is that the variables he wants to pass into useMutation aren't available at the time that the hook runs (for example, they may be dependent on a query completing). This means that code like this:

module MyMutation = [%graphql {|
   mutation doSomething($variable: String!) {
      doSomething(variable: $variable) {
         id
         name
         field
      }
   }
|}];

[@react.component]
let make = () => {
   let (result, executeMutation) = Hooks.useMutation(~request=MyMutation.make(~variable=/* What happens if variable isn't defined yet? */, ()));
}

has to apply an empty or fallback variable of some kind until variable becomes available. The goal is that we should basically never have to apply a variable at render time, only when executeMutation is called. That's the problem I can't quite wrap my head around fixing. I suppose we could have a user pass us MyMutation.make and call it with empty variables to get access to the parse function --> so basically we do what Robin's doing now on the library side rather than on the user side.

I also have an additional functor API worked out for all three hooks locally, so I'll push up that diff tomorrow so we can look at it and discuss. I like the idea of a useDynamicMutation, just can't quite figure out how we get the type inference right with our current setup.

[parkerziegler]

^ Here's the PR with the proposed functor API. @Schmavery if you're able to cook up a branch or PR for useDynamicMutation I'd ❤️ to see it.

[Schmavery]

@parkerziegler we should be able to do an API like this if people want, I have some code that seems to be typechecking locally mocked up for it

module MyMutation = [%graphql {|
   mutation doSomething($variable: String!) {
      doSomething(variable: $variable) {
         id
         name
         field
      }
   }
|}];

[@react.component]
let make = () => {
   let (result, executeMutation) = Hooks.useDynamicMutation(~query=MyMutation.query, ~parse=MyMutation.parse));

  // later...
  executeMutation(MyMutation.make(~var1="hello", ~var2="world", ()))
}

It's a little more verbose but allows everything to be typed without using functors (I noticed in your PR that it seems like the vars are an untyped Js.Json.t? Unless I misread)

It's kinda funny the limitations we're under because of needing to bind to the js API cause I can't imagine either the query or the parse function needing to be used before the vars are received... it feels like it would almost be easier to reimplement the hook on the reason side.

---

Another thing I was considering involved a PR to graphql_ppx_re to add a record on the MyMutation module containing the query, parse fn, and a fn of type varsType => unit (just to use to grab the type of the Js.t of the vars). Then you could have this api...

let (result, executeMutation) = Hooks.useDynamicMutation(MyMutation.definition);
executeMutation({"var1": "hello", "var2": "world"})

Cutest API but maybe too much work to get upstreamed haha

[robinweser]

@Schmavery Oh I really like that proposed useDynamicMutation! Especially with the typed vars etc. Don't like the Js.Dict syntax too much in general.

Also on the custom parse stuff: Yeah it's quite straight-forward to just use a => a, but you'll loose convenience e.g. null -> Option conversion (90% of our fields are nullable due to how our auth works).

[parkerziegler]

@Schmavery Ok, I think I'm seeing it now! I think for some reason I didn't think you could access .parse off the graphql_ppx_re module, but of course you can since that's literally what the functor is doing 🤦‍♂ If you'd like to PR the first useDyanmicMutation you have mocked up here I think that'd make a great API to start with. If your work upstream in graphql_ppx_re does end up getting merged we can migrate to this indeed quite cute API 💅 Let me know what you need from me! cc/ @robinweser @gugahoa does that sound good to you?

[parkerziegler]

Closed by #130 and will be released in v1.4.0!