Typed useMutation #69
Conversation
|
There are probably some bits of feedback from #68 that I should migrate to this PR when I get a chance, but I'd still be curious to get opinions on the general api |
|
|
||
| let (_, executeTreatMutation) = | ||
| Hooks.useMutation( | ||
| ~request={ |
There was a problem hiding this comment.
Is the idea here to show a way to use useMutation without graphql_ppx? If so, I like it!
src/hooks/UrqlUseMutation.re
Outdated
| let useMutationResponseToRecord = (result: useMutationResponseJs('a)) => { | ||
| let data = result->dataGet; | ||
| let useMutationResponseToRecord = | ||
| (parse: Js.Json.t => 'response, result: useMutationResponseJs('a)) => { |
There was a problem hiding this comment.
Similar to how we did things on useQuery, I believe useMutationResponseJs('a) can lose the type parameter and we can just type data as Js.Json.t.
src/hooks/UrqlUseMutation.re
Outdated
| let data = result->dataGet; | ||
| let useMutationResponseToRecord = | ||
| (parse: Js.Json.t => 'response, result: useMutationResponseJs('a)) => { | ||
| let data = |
There was a problem hiding this comment.
Also similar to useQuery let's opt for Belt.Option.map like @gugahoa suggested rather than this switch.
src/hooks/UrqlUseMutation.re
Outdated
| let error = result->errorGet; | ||
| let fetching = result->fetchingGet; | ||
|
|
||
| let response: UrqlTypes.response('a) = | ||
| let response: UrqlTypes.response('response) = | ||
| switch (fetching, data, error) { | ||
| | (true, _, _) => Fetching |
There was a problem hiding this comment.
One more "similar to useQuery" item – I liked how you stripped specifying the type here and instead supplied UrqlTypes.Fetching on the first match of the pattern matching (L37) to locally open that variant type. Let's opt for that here as well 🙌
src/hooks/UrqlUseMutation.re
Outdated
| }, | ||
| ) => { | ||
| let (useMutationResponseJs, executeMutation) = | ||
| useMutationJs(~query=request##query); |
There was a problem hiding this comment.
Since we only have a single parameter for useMutationJs, do you mind switching that external to not use a labelled argument for query? I know I had it that way originally 😅, but I don't see much use in it now.
There was a problem hiding this comment.
Overall I really like this @Schmavery! I think the API is really nice and it matches useQuery well. If you have a sec to apply some of the lessons learned there to this PR I feel comfortable merging it!
|
@parkerziegler I think that's everything |
The useMutation-counterpart of #68.
I wanted to see if the easy change for useQuery could work for useMutation as well. For the most part, it was straightforward, but I had to make a second little change to the api because the Module.make() function (the one generated by graphql_ppx that returns the {"query", "variables", "parse"} object) requires the variables to be passed in when you call it rather than as a payload to the executeMutation call.
The downside of this is that it changes the useMutation interface between the component and the hook and it's not the same interface as the js urql. One workaround we could do is passing the makeWithVariables function itself to useMutation so that usage would look like this:
I wasn't sure which design was better so just I went with one of them in the PR.
I noticed that the queries in 3-mutation weren't using graphql_ppx so I converted one of them to use it, and left another as an example of how this api could be used if you didn't want a dependency on graphql_ppx. It's a little worse than before I guess, but not terrible, I thought.