Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mutation optional variables #59

Open
Coobaha opened this issue Oct 6, 2018 · 3 comments
Open

Mutation optional variables #59

Coobaha opened this issue Oct 6, 2018 · 3 comments
Labels
api-breaking-change This feature/bug requires a breaking API change and a new major version bug

Comments

@Coobaha
Copy link

Coobaha commented Oct 6, 2018

Hi @mhallin,

I have a question about forcing None values inside mutation variables as null. Is this correct?

[%expr fun v -> match v with None -> Js.Json.null | Some v -> [%e child_parser] v] [@metaloc loc]

I think it will be more correct to treat None as undefined, and Some(Js.Nullable.null) as null inside make function.

itemInput = {
  id: Int, // <----- Optional
  name: String!
};
mutation createItem($input: itemInput!) 

with CreateItem.make(~item={ "id": None, name: "foo" })" request payload will be {item: { id: null, name: "foo"}}.

if None will be treated as undefined - JSON.stringify will drop keys from request payload. I guess this can be tricky since Js.Json.t is used and undefined is not a JSON of course. We can filter them out then?

@baransu
Copy link
Contributor

baransu commented Oct 15, 2018

There is no way to replace Js.Json.null with undefined. According to glennsl/bs-json#50 (comment) solution would be to omit null value in field_array somewhere here

[%expr Js.Json.object_ (Js.Dict.fromArray [%e field_array])] [@metaloc loc]

and remove all null values from the output JSON.

I understand how it should work but code generation is a little bit magical to me thus I have no idea how to implement it. Maybe @mhallin could help.

@mhallin
Copy link
Owner

mhallin commented Oct 28, 2018

This sounds similar to #26, where the conclusion would be to use option(option(t)) for optional input variables. I'm thinking now that Js.Nullable would be a better solution, though. Either way, this is a breaking change.

@mhallin mhallin added bug api-breaking-change This feature/bug requires a breaking API change and a new major version labels Oct 28, 2018
@Coobaha
Copy link
Author

Coobaha commented Oct 28, 2018

+1 for Js.Nullable it also aligns well with Bucklescript 4.0.0 runtime representation for optionals.

None -> undefined
Some(null) -> null
Some("string") -> "string"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-breaking-change This feature/bug requires a breaking API change and a new major version bug
Projects
None yet
Development

No branches or pull requests

3 participants