Proposed API change corrections

[jfrolich]

Reverting the reverted commit (Work in progress)

• [x] skip parameter
• [x] errorPolicy parameter
• [x] polInterval parameter

Let me know if there is anything else to fix as this is a pretty big change to the API surface.

Previous PR: https://github.com/Astrocoders/reason-apollo-hooks/pull/49

[MargaretKrutikova]

Thanks a lot for your work!

I wonder whether it would be possible to do the file rename in another PR? It would make it so much easier to review if we could compare with the previous useQuery and useMutation 🙂 Just a suggestion, what do you and others think? Would love some input from other reviewers!

I think we will need to rethink the incompleteMutation approach, it might be confusing for people just starting to use the library. I will experiment with it a bit more during the weekend 🙂

[jfrolich]

I think we will need to rethink the incompleteMutation approach, it might be confusing for people just starting to use the library. I will experiment with it a bit more during the weekend 🙂

Indeed. I couldn't find a better way without changing the javascript code. Looking forward to your input!

[jfrolich]

I wonder whether it would be possible to do the file rename in another PR? It would make it so much easier to review if we could compare with the previous useQuery and useMutation 🙂 Just a suggestion, what do you and others think? Would love some input from other reviewers!

Because of the revert-revert I don't know of a way to do this. Any ideas? At least to see the diff it is still possible to diff the two different files using a diff tool.

[fakenickels]

Because of the revert-revert I don't know of a way to do this. Any ideas? At least to see the diff it is still possible to diff the two different files using a diff tool.

Indeed! I think was just a problem with git itself, the change was so big it couldn't detect automatically that the files are still the same bot moved and changed.

I was talking with @MargaretKrutikova and what I'm gonna do is to create .rei files for the current API so we can detect better the breaking changes or regressions

[fakenickels]

I think we will need to rethink the incompleteMutation approach, it might be confusing for people just starting to use the library. I will experiment with it a bit more during the weekend 🙂

FYI Me too!

[jfrolich]

I actually found a way to remove the need of incompleteMutation with a no-op mutation (which is never run).

[jfrolich]

I was talking with @MargaretKrutikova and what I'm gonna do is to create .rei files for the current API so we can detect better the breaking changes or regressions

Good one. But since the whole API changed it's going to be a large breaking change. As far as options it probably good to go through the changes after the first commit of this PR (September 18), and see everything has been changed in this branch as well.

[jfrolich]

This is the diff output of

git diff f5e4096683bd8ba0e4662ee2d9c41e35492fd867..2e8a7055e79fdab88b20409b864f456e91f57f69

index f9d071d..b9f722c 100644
--- a/README.md
+++ b/README.md
@@ -120,6 +120,26 @@ Using `fetchPolicy` to change interactions with the `apollo` cache, see [apollo
 let (_simple, full) = UserQuery.use(~fetchPolicy=NetworkOnly, ());

+Using errorPolicy to change how errors are handled, see apollo docs. + +reason +let (simple, _full) = UserQuery.use(~errorPolicy=All, ()); + + +Using skip to skip query entirely, see apollo docs. + +```reason +let (simple, _full) =

• UserQuery.use(
• ~skip=
• switch (val) {
• | None => true
• | _ => false
• },
• (),
• ); +```

• useMutation

  
  diff --git a/commitlint.config.js b/commitlint.config.js
  new file mode 100644
  index 0000000..e89caf4
  --- /dev/null
  +++ b/commitlint.config.js
  @@ -0,0 +1,32 @@
  +module.exports = {

• rules: {
• 'body-leading-blank': [1, 'always'],
• 'footer-leading-blank': [1, 'always'],
• 'header-max-length': [2, 'always', 72],
• 'scope-case': [2, 'always', 'lower-case'],
• 'subject-case': [
• 2,
• 'never',
• ['sentence-case', 'start-case', 'pascal-case', 'upper-case']
• ],
• 'subject-empty': [2, 'never'],
• 'subject-full-stop': [2, 'never', '.'],
• 'type-case': [2, 'always', 'lower-case'],
• 'type-empty': [2, 'never'],
• 'type-enum': [
• 2,
• 'always',
• [
• 'build',
• 'chore',
• 'ci',
• 'docs',
• 'feat',
• 'fix',
• 'perf',
• 'refactor',
• 'revert'
• ]
• ]
• } +}; diff --git a/package.json b/package.json index f1e8f21..1ebfdd9 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "reason-apollo-hooks",
• "version": "2.5.1",
• "version": "2.6.0", "scripts": { "build": "bsb -make-world", "start": "bsb -make-world -w", @@ -14,7 +14,11 @@ "license": "MIT", "devDependencies": { "@apollo/react-hooks": "^3.0.0",
• "@commitlint/cli": "^8.2.0",
• "@commitlint/config-conventional": "^8.2.0", "bs-platform": "^5.0.6",
• "husky": "^3.0.5",
• "lint-staged": "^9.4.0", "reason-apollo": "^0.17.0", "reason-react": ">=0.7.0" }, @@ -24,5 +28,17 @@ "react-dom": "^16.8.6", "reason-apollo": "^0.17.0", "reason-react": ">=0.7.0"
• },
• "lint-staged": {
• "*.{re,rei}": [
• "bsrefmt --in-place -w 80",
• "git add"
• ]
• },
• "husky": {
• "hooks": {
• "pre-commit": "lint-staged",
• "commit-msg": "commitlint -E HUSKY_GIT_PARAMS"
• } } } diff --git a/src/Mutation.re b/src/Mutation.re index 4322878..33cb2af 100644 --- a/src/Mutation.re +++ b/src/Mutation.re @@ -9,7 +9,7 @@ type graphqlErrors; type error = { . "message": string,
• "graphlErrors": graphqlErrors,

• "graphqlErrors": graphqlErrors, };

  type refetchQueries = @@ -29,7 +29,7 @@ type controlledResult('a) = { error: option(error), };

-type controledVariantResult('a) = +type controlledVariantResult('a) = | Loading | Called | Data('a) @@ -72,7 +72,7 @@ module Make = (Config: Config) => { ~awaitRefetchQueries: bool=?, unit ) =>

• Js.Promise.t(controledVariantResult(Config.t));

• Js.Promise.t(controlledVariantResult(Config.t));

  [@bs.module "@apollo/react-hooks"] external useMutation: diff --git a/src/Query.re b/src/Query.re index 3c4a786..a0595e6 100644 --- a/src/Query.re +++ b/src/Query.re @@ -61,6 +61,12 @@ module Make = (Config: Config) => { notifyOnNetworkStatusChange: bool, [@bs.optional] fetchPolicy: string,

• [@bs.optional]

• errorPolicy: string,

• [@bs.optional]

• skip: bool,

• [@bs.optional]

• pollInterval: int, };

  [@bs.module "@apollo/react-hooks"] @@ -84,6 +90,9 @@ module Make = (Config: Config) => { ~client=?, ~notifyOnNetworkStatusChange=?, ~fetchPolicy=?,

• ~errorPolicy=?,

• ~skip=?,

• ~pollInterval=?, (), ) => { let jsResult = @@ -94,6 +103,9 @@ module Make = (Config: Config) => { ~client?, ~notifyOnNetworkStatusChange?, ~fetchPolicy=?fetchPolicy->Belt.Option.map(Types.fetchPolicyToJs),

• ~errorPolicy=?errorPolicy->Belt.Option.map(Types.errorPolicyToJs),

• ~skip?,

• ~pollInterval?, (), ), ); @@ -133,14 +145,18 @@ module Make = (Config: Config) => { [|jsResult|], );

• (

• switch (result) {

• | {loading: true} => Loading

• | {error: Some(error)} => Error(error)

• | {data: Some(data)} => Data(data)

• | _ => NoData

• },

• result,

• );

• let simple =

• React.useMemo1(

• () =>

• switch (result) {

• | {loading: true} => Loading

• | {error: Some(error)} => Error(error)

• | {data: Some(data)} => Data(data)

• | _ => NoData

• },

• [|result|],

• );

• (simple, result); }; -}; \ No newline at end of file +}; diff --git a/src/Types.re b/src/Types.re index fe7d418..fa7c38a 100644 --- a/src/Types.re +++ b/src/Types.re @@ -11,7 +11,7 @@ type networkStatus = | Error | Unknown;

-let toNetworkStatus = (status: Js.Nullable.t(int)) => { +let toNetworkStatus = (status: Js.Nullable.t(int)) => switch (status->Js.Nullable.toOption) { | Some(1) => Loading | Some(2) => SetVariables @@ -22,24 +22,39 @@ let toNetworkStatus = (status: Js.Nullable.t(int)) => { | Some(8) => Error | _ => Unknown }; -};

/**

• apollo-client/src/core/watchQueryOptions.ts */ type fetchPolicy = | CacheFirst
  • | CacheAndNetwork | NetworkOnly | CacheOnly | NoCache | Standby;

-let fetchPolicyToJs = fetchPolicy => { +let fetchPolicyToJs = fetchPolicy => switch (fetchPolicy) { | CacheFirst => "cache-first"

• | CacheAndNetwork => "cache-and-network" | NetworkOnly => "network-only" | CacheOnly => "cache-only" | NoCache => "no-cache" | Standby => "standby" }; -};

• +/**

• • apollo-client/src/core/watchQueryOptions.ts
• */ +type errorPolicy =
• | None
• | Ignore
• | All;

• +let errorPolicyToJs = errorPolicy =>

• switch (errorPolicy) {
• | None => "none"
• | Ignore => "ignore"
• | All => "all"
• };

[MargaretKrutikova]

Could you remove the diff from yarn.lock from the comment? It is not possible to read the conversation in this thread otherwise. Sorry, I had to mark it as off-topic to avoid endless scrolling 😄

[Qziem]

First of all i think changes in this PR are greate, it will significantly simplify usage.

I actually found a way to remove the need of incompleteMutation with a no-op mutation (which is never run).

I noticed that incompleteMutation is still in README.md

[jfrolich]

Could you remove the diff from yarn.lock from the comment? It is not possible to read the conversation in this thread otherwise. Sorry, I had to mark it as off-topic to avoid endless scrolling 😄

Done

[jfrolich]

I noticed that incompleteMutation is still in README.md

I forgot to save before committing. Resolved with the last commit.

[jfrolich]

Actually I have an idea to have two useMutation functions, one with a fixed mutation and one that allows the mutation to be passed to the resulting function. This allows the type system to better validate the arguments.

[jfrolich]

Now with useMutation and useDynamicMutation, in the first version you have to supply the mutation (and variables) in the hook, the second you supply the mutation (and variables) in the returned function of the hook. It is now typechecked so no exceptions when you forget to pass something in.

[Qziem]

About useDynamicMutation - it returns a function without any query assign. For example you name the function: setUser, and pass to it something diffrent (for example adress) and it will work. Im just wondering if it will be good ? In react apollo you pass query to useMutation and it returns function which only takes variables, so here there is no possible to do mismatch which i wrote above.

[fakenickels]

Some points for me so far:

• useDynamicMutation feels a bit hacky adds to much to the code as they are supposed to be a binding to Apollo with a thin layer on top
• ApolloHooks* prefix in the files should be dropped. We should use namespace: true in bs-config.json instead
• As the mutation/query hooks have trailing unit we could replace it for the graphql_ppx content and make the API look a bit easier.

Also to avoid the whole thing of passing the variables upfront, we could use first-class modules and pass the whole module as a param.

module UserQuery = [%graphql ...]
useQuery(~variables=UserQuery.make(~age)##variables, (module UserQuery))

So we don't the need to have a useDynamicMutation and the bindings would map more close to 1:1 against react-apollo JS.

Given that I'm second guessing if all this effort is worth it just to avoid using functors 😅 . A strong point could be not needing a functor instance all the time in runtime.

I'm gonna push this change soon so you can give feedback.

Also I'm thinking in sending a PR to graphql_ppx_re that would expose the type variablesObject so we could do an usage like

useQuery(~variables={"age": 12}, (module UserQuery))

[fakenickels]

Remind that this is still all a proposal! I did not got the code running in example yet, just made sure the compiler got happy.

Let me know your feedback everyone. And thanks a lot for your effort so far @jfrolich

[jfrolich]

• useDynamicMutation feels a bit hacky adds to much to the code as they are supposed to be a binding to Apollo with a thin layer on top

I think there is an advantage for two function signatures so that we can correctly typecheck the input and output function (the non dynamic needs variables up front and the dynamic only has a required variable argument in the returned function). I do agree that the implementation can be cleaner. I copy pasted a bunch of code, but that's an implementation detail.

It might just be better to bite the bullet and pass in UserMutation##query and UserMutation##parse for the dynamic mutation use-case, with variables being passed later.

ApolloHooks* prefix in the files should be dropped. We should use namespace: true in bs-config.json instead

We can't expose the useQuery / useMutation functions on the main module as far as I know if we use namespace: true.

As the mutation/query hooks have trailing unit we could replace it for the graphql_ppx content and make the API look a bit easier.

Why is a trailing unit an issue? All reason functions that have optional arguments need a trailing unit, so I think everyone is familiar with that.

Also to avoid the whole thing of passing the variables upfront, we could use first-class modules and pass the whole module as a param.

Actually passing in variables upfront is very nice in most use-cases (where the variables are coming from state/props), but we also need the dynamic use case.

module UserQuery = [%graphql ...]
useQuery(~variables=UserQuery.make(~age)##variables, (module UserQuery))

That is interesting even better with makeVariables exposed in graphql_ppx. I think it might be better to have it as the first argument to make it more consistent with react-apollo. Didn't know about this first class modules, don't know the implications and bucklescript output but sure looks like a good fit for this API!

[fakenickels]

We can't expose the useQuery / useMutation functions on the main module as far as I know if we use namespace: true.

That's a fair point, it makes the usage a bit shorter indeed. ApolloHooks.useQuery is better than ApolloHooks.Query.use. I agree in keeping it this way.

Why is a trailing unit an issue? All reason functions that have optional arguments need a trailing unit, so I think everyone is familiar with that.

Ah, it's not a problem with the trailing unit. What I mean is that it is not necessary as we can use the module as our positional argument instead of a trailing unit. Reducing the number of arguments needed. The only purpose of the trailing unit in Reason/OCaml in a function with optional params is to tell the compiler that you are done applying this function as soon as you pass the first positional argument after the optional ones.

Actually passing in variables upfront is very nice in most use-cases (where the variables are coming from state/props), but we also need the dynamic use case.

Yes, with this new usage you can have both cases in the same hook. Making the API even closer to the React Apollo one.

I think it might be better to have it as the first argument to make it more consistent with react-apollo

This is again my point about moving the module argument to the end to not need two positional arguments in different places. Let me know you guys feedback here after the explanation, if it is a good trade in favor of simplifying the usage.

Didn't know about this first class modules, don't know the implications and bucklescript output but sure looks like a good fit for this API!

Glad you liked! I was afraid of adding too much complexity just to make the usage a bit shorter. But as long as we keep the additional typing complexity hidden from the user for me it's fine. About the output, thanks to BS 5.2 the module is passed as an object in runtime

function FilterByNameCache(Props) {
  var name = Props.name;
  var match = ApolloHooks.useQuery(undefined, Caml_option.some(makeWithVariables({
                name: "2"
              }).variables), undefined, undefined, undefined, undefined, undefined, {
        query: ppx_printed_query,
        parse: parse
      });

[MargaretKrutikova]

I am sorry I wasn't participating for quite a while, was away on a short trip 😄
I think the new API with passing a module looks really good. I fixed a couple of things to make the example work again, I hope it is okay that I did it directly on the branch!

@jfrolich I left a couple of comments regarding small things, great work overall! I think we should go with the new api of passing modules, it seems like it solves all our issues 😄 Would love to hear some feedback from others too!

Edit: I assume we need to fix useSubscription the same way if we decide to go with the proposed API?

[jfrolich]

This is again my point about moving the module argument to the end to not need two positional arguments in different places. Let me know you guys feedback here after the explanation, if it is a good trade in favor of simplifying the usage.

Not sure about this. I think it's more straightforward to have it as the first argument, but from an ocaml standpoint I can see why it makes sense as the last argument. However I do prefer it to be the first argument as it's not a clear convention, and it keeps the API more in line with react-apollo.

[jfrolich]

I am sorry I wasn't participating for quite a while, was away on a short trip 😄 I think the new API with passing a module looks really good. I fixed a couple of things to make the example work again, I hope it is okay that I did it directly on the branch!

Great you did it in the branch! Thanks!

[jfrolich]

Very nice. I think we have something good here. @fakenickels, do you want to complete the PR? (update docs etc.). Also one more change would be to update useSubscription as well.

[jfrolich]

I was thinking that if we have the (first class) query module as the first argument we can use currying to have the variables as the later arguments for useQuery. For mutation we can do something similar (especially if we have makeVariables exposed: https://github.com/baransu/graphql_ppx_re/pull/35). Any thoughts?

[fakenickels]

Very nice. I think we have something good here. @fakenickels, do you want to complete the PR? (update docs etc.). Also one more change would be to update useSubscription as well.

Yes please! 🙏 much appreciated

I was thinking that if we have the (first class) query module as the first argument we can use currying to have the variables as the later arguments for useQuery.

I don't think it's a good idea like that because then we are not "fully applying" the hook call and we risk breaking the Rules of Hooks. The correct way to change the variables for later would be with refetchQuery.

[fakenickels]

@jfrolich after updating useSubscription and updating the project inside examples/ we are good to go for me!

[jfrolich]

I don't think it's a good idea like that because then we are not "fully applying" the hook call and we risk breaking the Rules of Hooks. The correct way to change the variables for later would be with refetchQuery.

Ah yeah that was also my concern. We run into the risk of people that do not fully apply and apply in an incorrect place.

[MargaretKrutikova]

I think there were still some unresolved comments, could you please look into them, @jfrolich ?

[jfrolich]

I think there were still some unresolved comments, could you please look into them, @jfrolich ?

Yes. Let me find some time and resolve the comments and wrap up the other stuff.

[Qziem]

Yes. Let me find some time and resolve the comments and wrap up the other stuff.

@jfrolich just ping about this :) I don't want to be rude, of course you will handle this when you will have time, I'm just waiting for this merge and release to npm 😄

[jfrolich]

I know. I’m a bit slammed at work at the moment. Will try to find time ASAP!

[MargaretKrutikova]

I will have time later this week, so I will help with whatever is left, so no stress, @jfrolich 😄 I also want to get this merged, I think it is going to be a huge improvement.

[jfrolich]

As far as I know everything should be included now. Let me know if there are any issues. Let's get this merged! 🎉

[fakenickels]

Amazing @jfrolich ! Thanks a lot, we'll review soon!

[Schmavery]

It's slightly hard to follow the thread here, but I figured I'd mention that we recently added a useDynamicMutation hook to reason-urql https://github.com/FormidableLabs/reason-urql/pull/130

We had a eerily similar discussion around functors, first-class modules, changing the ppx, etc and landed on an API like this for now:

let (response, executeMutation) = Hooks.useDynamicMutation(
      ~query=MyMutation.query,
      ~parse=MyMutation.parse,
    );
...
executeMutation(MyMutation.make(~var1="hello", ~var2="world"));

We implemented a couple of different options before deciding that functors and fcm were too much overhead for newcomers when they can be replaced by passing in another regular argument.

If I had the time to edit the ppx, I think we could have also gotten an api like this, which would have been cool too.

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

[jfrolich]

Hi @Schmavery, we had similar ideas but landed in passing the module using first class modules (a language feature I was not aware of, but perfect for this use case). I PR’d makeVariables to graphql_ppx_re, so when that lands we have a nice API to just create the variables.

Edit, sorry, half-read your reaction and typed a reply on my iPhone. Saw you discussed first class modules already 🤷‍♂️😅

[jfrolich]

If I had the time to edit the ppx, I think we could have also gotten an api like this, which would have been cool too.

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

I had a similar idea, and discussed it with @baransu (https://github.com/baransu/graphql_ppx_re/issues/33) graphql_ppx_re. As we have first class modules to pass the data of the module (with no runtime cost), I agree that we only really need makeVariables. definition is nice, but it's only necessary if you'd like an API without passing in the module itself (perhaps first class modules make it a bit less beginner friendly).

[jfrolich]

Are we ready to merge this @fakenickels, @MargaretKrutikova, @kamilkuz? 🚀

[fakenickels]

hey there @jfrolich will finish the final review today! sorry the delay

[fakenickels]

saw that your PR to graphql_ppx_re got merged too @jfrolich that's really nice, we can totally explore that now as well. I'm a bit busy today in the morning and partially in the afternoon but I'll do my best to get this into master today!

[fakenickels]

@Schmavery about the FCM discussion it was getting a bit philosophical for me, if it really adds more burden to beginners or if we are keeping them away from a cool lang feature haha. Anyways I really liked the usage with that definition tuple. We wouldn't even need to have a special useDynamicMutation here in our bindings.

[sgrove]

I had a chance to hack a bit with @Schmavery yesterday and we made a small (+18LoC) addition to graphql_ppx_re (just merged and released) and built a definition-based API for reason-urql that I think would work perfectly well for apollo-hooks as well, and it uses just basic tuples and currying.

Here's an example of the API differences: And the definition-based api applied to the other operation types: FormidableLabs/reason-urql#133 (comment)

let (response, refetch) = Hooks.useQuery(MyQuery.definition, ~var=1, ());
// With currying, the ~var=1 can be applied later at the call-site.
let (response, refetch) = Hooks.useMutation(MyMutation.definition, ~var=1, ());
let (response, execute) = Hooks.useDynamicMutation(MyMutation.definition);
let (response) = Hooks.useSubscription(MySub.definition, ~var=1, ());

This way we can avoid introducing any advanced type concepts, the api is (just about) as succinct and intuitive as possible, and it should compile down to (mostly) readable JS.

[jfrolich]

I had a chance to hack a bit with @Schmavery yesterday and we made a small (+18LoC) addition to graphql_ppx_re (just merged and released) and built a definition-based API for reason-urql that I think would work perfectly well for apollo-hooks as well, and it uses just basic tuples and currying.

Here's an example of the API differences: And the definition-based api applied to the other operation types: FormidableLabs/reason-urql#133 (comment)

let (response, refetch) = Hooks.useQuery(MyQuery.definition, ~var=1, ());
// With currying, the ~var=1 can be applied later at the call-site.
let (response, refetch) = Hooks.useMutation(MyMutation.definition, ~var=1, ());
let (response, execute) = Hooks.useDynamicMutation(MyMutation.definition);
let (response) = Hooks.useSubscription(MySub.definition, ~var=1, ());

This way we can avoid introducing any advanced type concepts, the api is (just about) as succinct and intuitive as possible, and it should compile down to (mostly) readable JS.

Love this! Especially the currying done in composeVariables, was already thinking if such a solution would be possible with currying so we don't need makeVariables in the API. @fakenickels, what do you think? I think it might be good to refactor to above API.

[jfrolich]

I had a chance to hack a bit with @Schmavery yesterday and we made a small (+18LoC) addition to graphql_ppx_re (just merged and released) and built a definition-based API for reason-urql that I think would work perfectly well for apollo-hooks as well, and it uses just basic tuples and currying.

Here's an example of the API differences: And the definition-based api applied to the other operation types: FormidableLabs/reason-urql#133 (comment)

let (response, refetch) = Hooks.useQuery(MyQuery.definition, ~var=1, ());
// With currying, the ~var=1 can be applied later at the call-site.
let (response, refetch) = Hooks.useMutation(MyMutation.definition, ~var=1, ());
let (response, execute) = Hooks.useDynamicMutation(MyMutation.definition);
let (response) = Hooks.useSubscription(MySub.definition, ~var=1, ());

This way we can avoid introducing any advanced type concepts, the api is (just about) as succinct and intuitive as possible, and it should compile down to (mostly) readable JS.

Actually I think passing the module would allow for a nicer compilation to javascript, and it allows for adding more data to the module later without breaking changes. (For instance the new serialize function). Then we only need composeVariables exposed on the module.

What about this api? (@sgrove, any other downsides to passing a (first class) module that I am not aware of?, it compiles really cleanly to JS). I think many people might not know about the particular language feature, but the syntax is pretty clear and you don't need to know all the details to be able to use it.

let (response, refetch) = Hooks.useQuery((module MyQuery), ~var=1, ());
// With currying, the ~var=1 can be applied later at the call-site.
let (response, refetch) = Hooks.useMutation((module MyMutation), ~var=1, ());
let (response, execute) = Hooks.useDynamicMutation((module MyMutation));
let (response) = Hooks.useSubscription((module MySub), ~var=1, ());

[jfrolich]

Hmm, thought some more about this, but there are extra parameters to pass into useQuery (such as client, fetchPolicy etc.). This means we can have the query variables after the query definition/first class module, and then after that a unit. However I think explicitly passing in variables as a labeled argument is a bit more explicit and clear (and also more in line with Apollo's API).

So all things considered in my opinion the API as in this PR is better for Apollo. I think the URQL API is also interesting, especially for a light implementation with a very simple API (and all the advanced options that Apollo offers).

We can consider to also pass in definition tuple instead of the (first class) module. I don't really care, I think both options are fine.

[jfrolich]

Hmm, thought some more about this, but there are extra parameters to pass into useQuery (such as client, fetchPolicy etc.). This means we can have the query variables after the query definition/first class module, and then after that a unit. However I think explicitly passing in variables as a labeled argument is a bit more explicit and clear (and also more in line with Apollo's API).

I was wrong we can mix the curried labeled arguments, so actually if the options don't collide with the variables, this might be a quite elegant solution. Experimenting with it right now but running into some issues with the type system.

[jfrolich]

As it looks now the API as proposed by @sgrove has a side effect that the variables have to be provided exactly in the order of the query. I reproduced this in their PR (https://github.com/FormidableLabs/reason-urql/pull/133#issuecomment-558903740). This is an important downside to be aware of. It makes the DX worse as people will not know why they get type errors for labeled arguments that normally can be in any position. Let's see if there is a solution to that. I don't completely grok why it is, but it's probably because of the polymorphic parametric types that the compiler doesn't have full information of the return type, and thus enforces the order of parameters. Any help appreciated of people that are more familiar with the intricacies of this!

[MargaretKrutikova]

Hmm, thought some more about this, but there are extra parameters to pass into useQuery (such as client, fetchPolicy etc.).

Does it mean that if we use composeVariables, it won't be possible to have query variables named the same way as those extra options passed into useQuery? That sounds like a disturbing limitation...

However I think explicitly passing in variables as a labeled argument is a bit more explicit and clear (and also more in line with Apollo's API).

I totally agree! I really like your addition with makeVariables, looks like the right way to go.

So maybe we can try out passing definition tuples instead of modules and leave makeVariables the way you implemented it, @jfrolich ? I have started trying out this new definition API and can look into it a bit more, if no one has done it yet 😄

Actually I think passing the module would allow for a nicer compilation to javascript, and it allows for adding more data to the module later without breaking changes. (For instance the new serialize function).

serialize can also be added to the definition later on, I think, and I agree that passing around modules might be a bit too much for someone just starting out with reason.. Let me know what you think!

[jfrolich]

Does it mean that if we use composeVariables, it won't be possible to have query variables named the same way as those extra options passed into useQuery? That sounds like a disturbing limitation...

I was wrong about it and it can be mixed with other labeled arguments, so that API would be pretty sweet. The only thing is that I found that the compiler doesn't accept the variables out of order due to the currying (I think). I pointed this out in the PR (https://github.com/FormidableLabs/reason-urql/pull/133#issuecomment-558903740).

I totally agree! I really like your addition with makeVariables, looks like the right way to go.

So maybe we can try out passing definition tuples instead of modules and leave makeVariables the way you implemented it, @jfrolich ? I have started trying out this new definition API and can look into it a bit more, if no one has done it yet 😄

I get the impression most people prefer Query.definition compared to FCM, so I agree it's probably better to use that instead of FCM (it's mostly a stylistic choice, I don't have a strong preference).

I'll wait for a bit if we can get the composeVariables currying working correctly. If that's technically not possible I will refactor to:

• Using definition tuple instead of FCM
• Keep using makeVariables (already in PR)

[fakenickels]

Both APIs look equally good and now the discussion is more about aesthetics so far, as I think no one could see clear disadvantages about one or the other.

So @jfrolich I think we could merge the PR as it is and let others use both libraries (urql and apollo-hooks) with this difference and see how it goes in the future. Probably a bit of difference will be good for now.

What you think?

cc @MargaretKrutikova @kamilkuz

Another point is to check if the demo is still working, can't trust hooks without checking in runtime unfortunately