Use "Config" module for default variables.

[nirvdrum]

I've recently encountered a vexing problem with the way default variables for a query are populated. At the core of it, these two queries are not equal:

let (simple, _full) = ListTodos.use()`

let (simple, _full) = ListTodos.use(~variables=ListTodosConfig.make()##variables, ());

The two queries are equivalent: they'll both fetch a list of todo items without passing any arguments to the query. However, to Apollo, they're two distinct queries. The first one, will appear as:

listTodos({})

While the latter will appear as:

listTodos({"filter":null,"limit":null,"nextToken":null})

The distinction is important for proper cache management. I ran into this mismatch problem when trying to use refetchQueries with a "create todo" mutation. Following the pattern in the example project, I had used something like:

CreateTodo.use(
  ~refetchQueries=
    _ => {
      let query = GraphQL.Queries.ListTodosConfig.make();
      [|ReasonApolloHooks.Utils.toQueryObj(query)|];
    }, ());

In this case, the usage of the "Config" module is required as far as I can tell. Unfortunately, that will fail to update the simple usage of ListTodos.use(). The query will be made during the refetch process, but Apollo will treat it as a new query and create a new cache entry rather than updating the existing one.

It looks like the problem is reason-apollo-hooks maps an options type from JS with BuckleScript and marks fields as "optional". If such fields are omitted, BuckleScript will also omit them (i.e., it will not populate the fields with default values). The "Config" modules, on the otherhand come from graphql_ppx and will always populate the fields with a default None/null values.

The solution for the time being is to always supply the ~variables keyword argument to the use call. However, beyond just being more verbose, I think this is an unexpected situation and can easily trip people up. Consequently, I think the better option is to update the definition of use to always use the "Config" module for generating default variables values.

[fakenickels]

Thanks for thoroughly explaining the problem.
That makes a lot of sense.

I think before making variables a required param we can try to find other solutions to make the bindings more correct.
Let's see what others will say about it

[nirvdrum]

Sorry if I confused the situation, but I'm not advocating for variables to be required. What I'm suggesting is that internally the "Config" module should be invoked if variables is None. I was hoping to open a PR for this, but I'm unable to get a built version of reason-apollo-hooks to work with my app. I get some error about a hook appearing outside a component function, but 2.5.1 works fine for me. I only get the error with my own built version (even based off the 2.5.1 tag). I'll need to look more into that separately.

In any event, I'm suggesting changing: https://github.com/Astrocoders/reason-apollo-hooks/blob/1c4d89563fa094eb7a116d8d59f14b4b04f0f885/src/Query.re#L83

to:

options(~variables=Belt.Option.getWithDefault(variables, Config.make()##variables), ~client?, ~notifyOnNetworkStatusChange?, ()),

Since I couldn't get that working locally, the suggested change may not be accurate, but I hope it illustrates my proposed change. variables would still be optional, but rather than passing None through to the options constructor when no variables value is supplied, the default values from the "Config" module would be used instead. Looking briefly at master, the same sort of change should be doable as well.