Add useDynamicMutation hook #130
Conversation
src/hooks/UrqlUseMutation.re
Outdated
|
|
||
| let executeMutation = | ||
| React.useCallback1( | ||
| varsObj => executeMutationJs(Some(varsObj##variables)), |
There was a problem hiding this comment.
A nit on naming here. This is the Js.t created by calling .make() on the graphql_ppx_re module, yeah? Could we use the same nomenclature we do in useMutation and name this request?
| varsObj => executeMutationJs(Some(varsObj##variables)), | |
| request => executeMutationJs(Some(request##variables)), |
There was a problem hiding this comment.
The type is slightly different (explained below), which is why I went with a different name. Doesn't bother me to change it though, if you think it's clearer.
There was a problem hiding this comment.
Ah that makes sense. I really wish we could destructure "open" objects like this so it'd be more like:
{ variables } => executeMutationJs(Some(variables))Still I do find request a little easier to understand. This object passed to this callback is the result of calling .make() on that graphql_ppx_re module, and we have been calling that request throughout the codebase. Even tho we're restricting its type a bit more, it's more or less the same thing.
There was a problem hiding this comment.
@Schmavery this is really excellent. @robinweser is this an API you'd be happy with? I really like it, and I like that it keeps us out of functor usage – I think that's a huge benefit of reason-urql. A few smol comments you can take / leave. Thanks so much! I'll leave this open if you want to make those changes, otherwise feel free to merge at your leisure.
|
Also, I applied the patch locally and it works great. Yeah, |
|
Updated the variable name and comments, will merge tomorrow morning if there are no more concerns by then (or feel free to beat me to it). |
|
This looks brilliant! Happy to test it out :) |
|
Looks great to me @Schmavery. I'll write up some docs for it this afternoon and get this released today or tomorrow! Thanks so much! |
Wrote up some code for the useDynamicMutation idea in #128
Updated the examples to show usage but had some trouble running them (webpack acting up) so it would be great if someone can double-check that it works, otherwise I'll figure it out some other time.