search

mhallin / graphql_ppx

GraphQL PPX rewriter for Bucklescript/ReasonML
BSD 3-Clause "New" or "Revised" License
320 stars 42 forks source link

Fix mutation optional variables #64

Open baransu opened 5 years ago

baransu commented 5 years ago

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: https://github.com/mhallin/graphql_ppx/issues/26 and https://github.com/mhallin/graphql_ppx/issues/59

baransu commented 5 years ago

@mhallin How can I push it forward?

mhallin commented 5 years ago

Thanks for this.

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

baransu commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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