Fix mutation optional variables

[baransu]

It turns out there is a way to filter out null values before generating variables JSON without breaking the API. Implementation allows to None to Js.Json.null which is not compatible with GraphQL spec but later we filter that values out before creating Js.Json.object_. It doesn't force usage of Js.Nullable nor option(option(t)) so it's not API breaking change. The whole implementation is done in BuckleScript related code and I'm not sure if those changes should be applied to the native part as well.

Should fix: #26 and #59

[baransu]

@mhallin How can I push it forward?

[mhallin]

Thanks for this.

It looks like it basically flips the default of converting None into null to undefined instead. Is this correct?

[baransu]

Yes.
Since there is no Js.Json.undefined we have to filter null out and it's easier to do after encoding into Js.Json.t.

[mhallin]

I'm a bit weary about this since it changes behaviour in a way that might break clients, so it might be better to just bite the bullet and introduce either option(option(t)) or Js.Null_undefined.t instead.

[baransu]

But with that change we’ll have correct behaviour matching the BuckleScript implementation (None is undefined) and GraphQL nullable in arguments is undefined not null, without need for API change. Since we’re not 1.x I think we should think about it as breaking change and make major release (so since we’re 0.x a minor change).

Maybe it’s good idea to talk about it on Discord and gather feedback from the users and also ask in reason-apollo because a lot of peoples comes from there.

/cc @Gregoirevda

[MoOx]

In short, would this PR avoid to have graphql server yelling if we send None value in a mutation when undefined is supposed to be accepted?

[baransu]

@MoOx In JSON there is not undefined AFAIK. Right now we send null which isn't correct because null should be omitted in variables instead of being sent as null. This PR removes the variables if they are None

[MoOx]

@baransu Yeah sorry, forgot that undefined isn't a thing in JSON. This PR does what I need: omit None values.
Would love to see that released.
@mhallin Can we do something about CI? error (ERROR while compiling dune.1.6.2) here seems unrelated to the PR.

[baransu]

@MoOx I've ported this fix into baransu/graphql_ppx_re if you need it.