Upgrade hooks api to use graphql-ppx 1.0 #225
Conversation
amiralies
commented
Nov 1, 2020
•
amiralies
commented
- useQuery
- useMutation
- useSubscription
- query examples
- mutation examples
- subscription examples
also removed redundant useDynamicMutation
|
Thanks for this @amiralies! I should be able to get to it this weekend and give a full review. Really appreciate you taking this on! |
examples/2-query/public/index.css
Outdated
| @@ -13,6 +13,7 @@ | |||
| border-radius: 5px; | |||
| box-shadow: 0 10px 15px -3px rgba(0, 0, 0, 0.1), | |||
| 0 4px 6px -2px rgba(0, 0, 0, 0.05); | |||
| margin-top: 15rem; | |||
There was a problem hiding this comment.
Any reason for this? Mind posting a screenshot of the changes? It looks like you altered the structure of the HTML below as well, is there a compelling reason to do this?
There was a problem hiding this comment.
Dex is positioned in center of page (notice scroll position):
I think correct way to handle this is to use overflow-y: scroll on list.
There was a problem hiding this comment.
I've updated layout using flexbox
src/hooks/UseMutation.re
Outdated
| let useMutation: | ||
| type data variables. | ||
| ( | ||
| ~query: (module Types.Operation with |
There was a problem hiding this comment.
I'd advocate for one of two options here:
- Rename this labelled argument to
mutation.
| ~query: (module Types.Operation with | |
| ~mutation: (module Types.Operation with |
- Don't bother with a labelled argument, since the hook only accepts a single argument.
1 is more compatible with the current API, while 2 maybe makes more sense from a language perspective. What do you think?
There was a problem hiding this comment.
Both compiles to same js.
I actually like the first approach for consistency between APIs.
AFAIK there shoudn't be any problem with unary function with a (explicitly typed) labeled argument.
you can even omit label (though compiler gives a warning):
let (state, exec) = Hooks.useMutation((module MyMutation));maybe @jfrolich can also help with this.
There was a problem hiding this comment.
I don't think it makes a difference. If you have multiple arguments it's good to have the query as the first argument because of type inference I think. Otherwise the variable record might not be inferred (not 100% sure).
src/hooks/UseMutation.re
Outdated
| type t = data and type t_variables = variables) | ||
| ) => | ||
| useMutationResponse(variables, data) = | ||
| (~query as (module Query)) => { |
There was a problem hiding this comment.
Let's name this locally like so:
| (~query as (module Query)) => { | |
| (~mutation as (module Mutation)) => { |
There was a problem hiding this comment.
This looks great @amiralies, a huge, huge thank you for all of this work. A few comments that are mostly small stylistic suggestions, but the changes are very very close. Ping me again when you've addressed the comments, I'll review one last time, and then we can merge this puppy. Thanks again 💯
|
@parkerziegler I think this is ready to be reviewed again 🚀 |
There was a problem hiding this comment.
Beautiful work @amiralies! Thank you again so much for moving this forward!
|
@all-contributors please add @amiralies for code, example. |
|
I've put up a pull request to add @amiralies! 🎉 |
