Unified definition

[sgrove]

The tuple representation allows for much slimmer, simpler usage by clients (no need for first-class modules, functors, etc.).

Here's an example of usage before/after this PR: https://github.com/FormidableLabs/reason-urql/pull/132/files#diff-58c974b09e3cc8d4b9e0332289157a63

And the definition-based api applied to the other operation types: teamwalnut/rescript-urql#133 (comment)

Right now this PR only provides it for bucklescript - if it looks good to you, I'll copy the implementation over to the native side as well.

[baransu]

I really like this change. Thanks you!

[MargaretKrutikova]

This is very nice, thanks so much! Opens up for many improvements in graphql client libraries 😄

Could you please explain what the last function in the tuple does? I see it is called composeVariables in reason-urql, but what exactly is its purpose (and also type)? Can't seem to figure it out from the changes in this PR, ppx is tough 😕
Thanks!

[sgrove]

I agree on ppx's being tough! It took an hour or so to set up a tight iteration loop to develop on it.

Here's what the tuple expands into with my test-case GraphQL query (with just a $key: ID! parameter/variable):

  let makeVariables = (~key, ()) =>
    Js.Json.object_(
      [|("key", Js.Json.string(key))|]
      |> Js.Array.filter(((_, value)) =>
           !Js.Json.test(value, Js.Json.Null)
         )
      |> Js.Dict.fromArray,
    );
  let definition = (
    parse,
    ppx_printed_query,
    (graphql_ppx_use_json_variables_fn, ~key, ()) =>
      graphql_ppx_use_json_variables_fn(
        Js.Json.object_(
          [|("key", Js.Json.string(key))|]
          |> Js.Array.filter(((_, value)) =>
               !Js.Json.test(value, Js.Json.Null)
             )
          |> Js.Dict.fromArray,
        ),
      ),
  );

So the last item in the tuple (composeVariables) is a higher-order function (that is substantially the same as makeVariables). If we ignore the first argument for now, all this function does is take labelled arguments and build the Js.Json.t variables object. Then it passes that variables object to graphql_ppx_use_json_variables_fn.

The idea here is that the client (apollo, urql, etc.) can pass in a function that takes a completed variables object and does whatever (probably makes a network call), and by the magic of currying, this HoF will turn it into a labelled-argument function.

Hopefully that makes sense - it took awhile to noodle through in person with @Schmavery!

[Schmavery]

@MargaretKrutikova in case it helps, you can also think of the last function as being one that takes a function f and arguments a and then returns f(a).

It's just a little weirder because we want to be able to pass the arguments as named arguments, so it makes the types a bit harder to read in the graphql client code.

This is useful because we can write whatever logic we want the client to do with the variables, and then shove that inside the composeVariables function and get back a new function that accepts all the variables as named args and then calls our function with those variables parsed into a Js.Json.t

[jfrolich]

This was merged quickly! Wouldn't it be better to allow for some discussion, and wait until it has the native code before releasing?

We are essentially duplicating data in the tuple, basically you now have the query definition and the parse function in two places. which might have some potentially negative side effects.

I think it's better to have one of these situations:

• Deprecate parse and query from the module, with a breaking change that everything is in definition now.

• Actually if an implementation wants to accept the "query/mutation definition" you can also use first class modules to pass the module contents, so then this PR can only add composeVariables (very nice addition BTW). See here for how a implementation could work with this: Proposed API change corrections reasonml-community/reason-apollo-hooks#59 (comment).

• More radical, just compile to the definition, and don't compile to a module anymore. This makes everything much easier because we can just easily pass queries and mutations to functions. When the definition is a record using it directly can also still be quite nice.

I'm also interested in how this API would work if extra things are added to the definition like a serialize function. (Will these new functions also be added in two places?).

[baransu]

@jfrolich I understand your concerns. Thank you for starting a discussion. I would like to answer a few of them.

---

We are essentially duplicating data in the tuple, basically you now have the query definition and the parse function in two places. which might have some potentially negative side effects.

We're not duplicating parse and query, we're just passing already defined functions. If you look into ppx output you'll see:

 let definition = (
    parse,
    ppx_printed_query,
    (graphql_ppx_use_json_variables_fn, /* variables */) => /* output */
)

---

Actually if an implementation wants to accept the "query/mutation definition" you can also use first class modules to pass the module contents, so then this PR can only add composeVariables (very nice addition BTW).

First-class modules are a quite advanced feature and harder concept to grasp. On one hand it can be a good idea to force people to learn about it but on the other hand it's harder to get started coming from JS background.

---

More radical, just compile to the definition, and don't compile to a module anymore.

This is interesting, I would really like to pay with that. It would allow us to remove this magic: https://github.com/baransu/graphql_ppx_re/blob/master/src/bucklescript/output_bucklescript_module.re#L30 but we would lose access parse return type in easy way as it's available via Query.t.

[MargaretKrutikova]

Thanks a lot for the explanation, @sgrove @Schmavery ! Is there any way we could have those changes for native too?

[nirvdrum]

Please consider adding something to the README, possibly with a migration guide. Currently that information is spread out in this comment thread and a 3rd party's repo for sample code changes.

[baransu]

@nirvdrum Created issue for that #55