Proposed API change #49

jfrolich · 2019-09-18T05:38:05Z

This is a WIP, but I found that the functor API created quite a bit of boilerplate code. When I looked at the source of reason-urql, it found a way to implement a more concise API, so I messed around with the source of reason-react-hooks if we can have something like that that with apollo and this is the result. I would like to get your opinion and possible improvements.

In short we can go from this:

module ActivityPhotosQueryConfig = [%graphql
  {|
  query ActivityPhotos($feedId: ID, $scaleFactor: Float!, $height: Float!) {
    feedImageList(feedId: $feedId) {
      nodes {
        id
        width
        height
        file(scaleFactor: $scaleFactor, height: $height) {
          url
        }
      }
    }
  }
|}
];
module ActivityPhotosQuery =
  ReasonApolloHooks.Query.Make(ActivityPhotosQueryConfig);

[@react.component]
let make = (~feedId=?, ~small=false, ~edit=false) => {
  let height = small ? 100. : 200.;
  let (query, _full) =
    ActivityPhotosQuery.use(
      ~variables=
        ActivityPhotosQueryConfig.make(
          ~feedId?,
          ~scaleFactor=PixelRatio.get(),
          ~height,
          (),
        )##variables,
      (),
    );
}

to this:

module ActivityPhotosQuery = [%graphql
  {|
  query ActivityPhotos($feedId: ID, $scaleFactor: Float!, $height: Float!) {
    feedImageList(feedId: $feedId) {
      nodes {
        id
        width
        height
        file(scaleFactor: $scaleFactor, height: $height) {
          url
        }
      }
    }
  }
|}
];


[@react.component]
let make = (~feedId=?, ~small=false, ~edit=false) => {
  let height = small ? 100. : 200.;
  let (query, _full) =
    ReasonApolloHooks.useQuery(
      ~query=
        ActivityPhotosQuery.make(
          ~feedId?,
          ~scaleFactor=PixelRatio.get(),
          ~height,
          (),
        ),
      (),
    );
}

Similar for mutations, however we need to pass in an initial mutation if we want to provide variables later:

let (create, _simple, _full) =
  ReasonApolloHooks.useMutation(
    ~incompleteMutation=CreateFeedImageMutation.query,
  (),
);

We can run the create function like this:

create(
  ~mutation=CreateFeedImageMutation.make(~feedImage, ()),
  ~refetchQueries=_ => [|refetchQuery(~feedId?, ())|],
  ~awaitRefetchQueries=true,
  (),
)

or we can add the mutation from the get go (and then we don't have to supply it later):

let (create, _simple, _full) =
  ReasonApolloHooks.useMutation(
    ~mutation=CreateFeedImageMutation.make(~feedImage, ()),
  (),
);

To be honest the mutation API is suboptimal. I think there is a way that we do not need to provide the incompleteMutation but this involves changing the javascript useMutation hook.
OR we need to modify graphql_ppx, to generate a different module.

fakenickels · 2019-09-19T16:44:36Z

Thanks for your proposal!
I think we can leave this opened for a while to see what others will say about it

jfrolich · 2019-09-20T08:34:03Z

Sounds great. I am pretty new to Reason/OCaml, so there are probably many things that can be better.

fakenickels · 2019-10-04T00:26:22Z

No opinions of otherss so far but in overall I liked the simplification.
Can you update the examples? After that I think we are good to go

…-hooks into api-change

jfrolich · 2019-10-08T10:12:30Z

I updated the PR, a few changes:

Changed the main module to ApolloHooks (debatable, but more concise it's possible now that the package is not namespaced anymore)
Updated graphql_ppx in the example to graphql_ppx_re (which is a maintained version of graphql_ppx in reason -- same API).
Removed the namespacing so we can do ApolloHooks.useQuery directly instead of ApolloHooks.Query.useQuery or something. Also have all the hooks available when opening ApolloHooks.

fakenickels · 2019-10-15T09:27:52Z

Thanks a lot @jfrolich ! These next upcoming weeks we'll be really busy for me but I'll try to review and merge ASAP

fakenickels · 2019-10-15T09:28:37Z

examples/persons/package.json

    "css-loader": "^3.2.0",
-    "graphql_ppx": "^0.2.8",
+    "@baransu/graphql_ppx_re": "^0.2.0-beta.2",


Yeah so cool to see activity again in this package. It has so much potential.

jfrolich · 2019-10-15T12:43:58Z

Thanks a lot @jfrolich ! These next upcoming weeks we'll be really busy for me but I'll try to review and merge ASAP

Cool. No worries. When this is merged I also have a pull-request that allows to do fetchMore in pure reason, instead of the raw bucklescript escape hatch!

sgrove · 2019-10-23T22:47:30Z

This looks fantastic!

We've added code-gen support for the current API, you can see the current usage for both queries and mutations here: https://serve.onegraph.com/short/B8DR68 (open code exporter and choose Reason as the language)

I'd love to get this merged in and make the code that much more approachable for workshops (and for me too of course!).

fakenickels · 2019-10-24T00:15:50Z

Amazing!! Will get this merged before weekend 🙏

jfrolich · 2019-10-24T01:58:21Z

This looks fantastic!

We've added code-gen support for the current API, you can see the current usage for both queries and mutations here: https://serve.onegraph.com/short/B8DR68 (open code exporter and choose Reason as the language)

I'd love to get this merged in and make the code that much more approachable for workshops (and for me too of course!).

Wow that codegen is awesome!

…-hooks into api-change

jfrolich · 2019-10-24T06:26:45Z

Amazing!! Will get this merged before weekend 🙏

I merged in the upstream changes and upgraded graphql_ppx_re in the example.

fakenickels · 2019-10-24T11:29:31Z

Merged will release soon!

fakenickels · 2019-10-24T11:34:52Z

Thanks lot @jfrolich for this!! Awesome work. And thanks everyone for giving feedback

MargaretKrutikova · 2019-10-24T18:22:05Z

I am a bit late to the party, but what happened to errorPolicy in useQuery? I think it disappeared.

fakenickels · 2019-10-24T18:22:37Z

Looks like we have some regressions here indeed, along with #57

MargaretKrutikova · 2019-10-24T18:22:38Z

src/ApolloHooksTypes.re

-/**
- * apollo-client/src/core/watchQueryOptions.ts
- */
-type errorPolicy =


Are those not used anymore?

Ah that is probably an upstream change that I forgot to merge in (diffs were not working anymore so I had to manually look up what has been added to master in the meantime)

fakenickels · 2019-10-24T18:23:02Z

We haven't released this just yet so there is still time to fix

MargaretKrutikova · 2019-10-24T18:26:13Z

I will try to run the example project locally, and see what I can fix 😄

MargaretKrutikova · 2019-10-24T18:56:21Z

src/ApolloHooksMutation.re

+          | (None, None) => raise(Error("Need to pass a mutation"))
+          };
+
+        React.Ref.setCurrent(parse, Some(mutation##parse));


Oh, this runs inside of useMemo? Callback passed into useMemo must be pure because it will run on every render, see react docs. And later it calls jsMutate which will send the actual query, is it correct?
I am not sure it is a good idea to implement it this way, I think we will have to rethink this approach a bit, what do you think, @fakenickels ?

Sorry, my bad, spent some time alazying the code and realised that it is memoizing the function and not actually calling it 🤦‍♀ 😄 Maybe it makes sense to use useCallback instead of useMemo here?

I merged this change from master, but good suggestion. useCallback is just a shortcut for memoizing functions with useMemo if I remember correctly.
(edit: it actually memoizes the result and doesn't memoize the function, so useCallback is not applicable here)

edit2:

Callback passed into useMemo must be pure because it will run on every render, see react docs.

It does not run on every render, it only runs if the dependencies change.

MargaretKrutikova · 2019-10-24T18:58:34Z

src/ApolloHooksMutation.re

+
+let useMutation =
+    (
+      ~incompleteMutation=?,


I am not sure incompleteMutation is the right name here, it might get really verbose and tedious to write on many mutations, can we come up with something more succinct for this case? 😄

Usually you would use the mutation argument, unless you need to set the arguments dynamically. It would be better to have the mutation as an optional argument, but that needs some changes on the javascript side (I think!). Anyway I agree it's not ideal (and hopefully temporarily). Let me know what better name you are thinking of!

MargaretKrutikova · 2019-10-24T18:59:59Z

src/ApolloHooksMutation.re

+        | (_, Some(incompleteMutation)) => incompleteMutation
+        | (None, None) =>
+          raise(
+            Error("You need to provide a mutation or an incomplete mutation"),


I think we need to have a better exception type, something more descriptive than Error, does it makes sense, @fakenickels ?

Maybe we can avoid throwing errors here by making sure the user can't make a mistake on the type level? I think it will be better to not throw errors if the type system can guarantee that the values are correct 😄 I was thinking about using some kind of a variant:

type mutationConfig('a) = Query(string) | Config('a);

and then have it in useMutation:

let useMutation = ( ~mutation: mutationConfig('a)=?, ~client=?, .... (), )

And remove incompleteMutation, maybe something in that direction could be a more intuitive API?

Ideally we can remove the need to pass in incompleteMutation in the future which will solve this issue.

fakenickels · 2019-10-24T20:19:19Z

I think will be better to revert this and open the PR again so we can make we have less breaking changes

fakenickels · 2019-10-24T20:20:32Z

@jfrolich would you mind reopening this PR?

jfrolich · 2019-10-25T02:00:20Z

Sorry about this. I could directly merge master because due to filename changes it wouldn't diff the fires. So I looked up the individual commits and applied them manually, but I messed up and missed two additions!

I will re-open this PR, and fix the issues as reported.

Some comments of @MargaretKrutikova are about existing code not related to this PR. This can be tackled in a separate PR. (Except the naming of incompleteMutation, happy to change this name if anyone has a name that is better suited or a way so we don't need to pass in the incomplete mutation.

fakenickels · 2019-12-15T16:02:26Z

@all-contributors add @jfrolich code, review and ideas

allcontributors · 2019-12-15T16:02:34Z

@fakenickels

I've put up a pull request to add @jfrolich! 🎉

jfrolich added 2 commits September 18, 2019 13:04

change of the API

f8bd39d

merge upstream changes

a818a0a

jfrolich added 4 commits October 8, 2019 17:46

Refactor and adjust examples

f97162e

Merge branch 'master' of https://github.com/Astrocoders/reason-apollo…

bb4ac45

…-hooks into api-change

Fix typo

57d1f2b

Update README.md

73303c3

fakenickels reviewed Oct 15, 2019

View reviewed changes

jfrolich added 3 commits October 24, 2019 14:19

Merge branch 'master' of https://github.com/Astrocoders/reason-apollo…

9e8f98c

…-hooks into api-change

Merge pull request reasonml-community#56

97a31c6

update graphql_ppx_re

492f26c

fakenickels merged commit 624ff24 into reasonml-community:master Oct 24, 2019

MargaretKrutikova reviewed Oct 24, 2019

View reviewed changes

fakenickels mentioned this pull request Oct 24, 2019

Revert "Proposed API change" #58

Merged

jfrolich mentioned this pull request Oct 30, 2019

Proposed API change corrections #59

Merged

3 tasks

fakenickels mentioned this pull request Oct 31, 2019

Add NotCalled to variant & remove Called from variant. #60

Merged

allcontributors bot mentioned this pull request Dec 15, 2019

docs: add jfrolich as a contributor #81

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposed API change #49

Proposed API change #49

jfrolich commented Sep 18, 2019 •

edited

Loading

fakenickels commented Sep 19, 2019

jfrolich commented Sep 20, 2019

fakenickels commented Oct 4, 2019

jfrolich commented Oct 8, 2019

fakenickels commented Oct 15, 2019 •

edited

Loading

fakenickels Oct 15, 2019

jfrolich Oct 15, 2019

jfrolich commented Oct 15, 2019

sgrove commented Oct 23, 2019

fakenickels commented Oct 24, 2019

jfrolich commented Oct 24, 2019

jfrolich commented Oct 24, 2019

fakenickels commented Oct 24, 2019

fakenickels commented Oct 24, 2019

MargaretKrutikova commented Oct 24, 2019

fakenickels commented Oct 24, 2019

MargaretKrutikova Oct 24, 2019

jfrolich Oct 25, 2019

fakenickels commented Oct 24, 2019

MargaretKrutikova commented Oct 24, 2019

MargaretKrutikova Oct 24, 2019 •

edited

Loading

MargaretKrutikova Oct 24, 2019 •

edited

Loading

jfrolich Oct 25, 2019 •

edited

Loading

MargaretKrutikova Oct 24, 2019

jfrolich Oct 25, 2019

MargaretKrutikova Oct 24, 2019

MargaretKrutikova Oct 24, 2019 •

edited

Loading

jfrolich Oct 25, 2019

fakenickels commented Oct 24, 2019

fakenickels commented Oct 24, 2019

jfrolich commented Oct 25, 2019 •

edited

Loading

fakenickels commented Dec 15, 2019

allcontributors bot commented Dec 15, 2019

Proposed API change #49

Proposed API change #49

Conversation

jfrolich commented Sep 18, 2019 • edited Loading

fakenickels commented Sep 19, 2019

jfrolich commented Sep 20, 2019

fakenickels commented Oct 4, 2019

jfrolich commented Oct 8, 2019

fakenickels commented Oct 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfrolich commented Oct 15, 2019

sgrove commented Oct 23, 2019

fakenickels commented Oct 24, 2019

jfrolich commented Oct 24, 2019

jfrolich commented Oct 24, 2019

fakenickels commented Oct 24, 2019

fakenickels commented Oct 24, 2019

MargaretKrutikova commented Oct 24, 2019

fakenickels commented Oct 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fakenickels commented Oct 24, 2019

MargaretKrutikova commented Oct 24, 2019

MargaretKrutikova Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

MargaretKrutikova Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

jfrolich Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MargaretKrutikova Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fakenickels commented Oct 24, 2019

fakenickels commented Oct 24, 2019

jfrolich commented Oct 25, 2019 • edited Loading

fakenickels commented Dec 15, 2019

allcontributors bot commented Dec 15, 2019

jfrolich commented Sep 18, 2019 •

edited

Loading

fakenickels commented Oct 15, 2019 •

edited

Loading

MargaretKrutikova Oct 24, 2019 •

edited

Loading

MargaretKrutikova Oct 24, 2019 •

edited

Loading

jfrolich Oct 25, 2019 •

edited

Loading

MargaretKrutikova Oct 24, 2019 •

edited

Loading

jfrolich commented Oct 25, 2019 •

edited

Loading