zth / rescript-relay

Use Relay with ReScript.
https://rescript-relay-documentation.vercel.app/docs/getting-started
338 stars 53 forks source link

Distinguishing between nulls and undefindes in variables #348

Closed mbirkegaard closed 1 year ago

mbirkegaard commented 2 years ago

Currently nullable variables in mutations and queries become optional on the rescript-relay side.

Suppose there's mutation that looks something like

input MyMytationInput {
  id: ID!
  a: Int
  b: Int
}

myMutation(input: MyMutationInput): string

which edits some thing. According to [coerce argument values](http://spec.graphql.org/October2021/#CoerceArgumentValues()) in the graphql spec, there's a difference between executing

mutation myMutation(input: {
  id: "my-id",
  a: 7,
  b: null
})

and

mutation myMutation(input: {
  id: "my-id",
  a: 7,
})

However, on the rescript-relay side the generated mutation input has type

type myMutationInput = {
  id: string,
  a: option<int>
  b: option<int>
}

This disallows sending an explicit null. Setting b: None when calling the mutation turns it into undefined. Our current solution to this problem is to have the resolver for the mutation turn undefined into null depending on the use case.

I think my preference is an explicit approach where all nullable variables are of type Js.Nullable.t<'value> and handled when serialising, but I understand that may not be ergonomic enough for everyone and that an escape hatch solution like https://github.com/jeddeloh/rescript-apollo-client/pull/27 is prefered.

zth commented 2 years ago

My initial hunch for this is having something similar to this:

mutation X($someVariable: SomeType!) @rescriptRelayAllowNull {

...which would make all variables in the operation modelled as Js.Nullable.t rather than option. It'd work for all operations, so queries as well.

Thoughts?

mbirkegaard commented 2 years ago

I think that could work. It would have to be nullables all way down to cover our concrete use case, where the nullable fields are on a deeper type.

Js.Nullable.t could work, but option<Js.Null.t> is also worth considering.

If the generated type is something like (for some new schema, not the one above)

module Types {
  ...
  type rec input {
     id: string
     inputA: inputA
  } and
  type inputA {
    a: int,
    b: option<Js.Null.t<int>>,
    c: option<Js.Null.t<int>>
  }
}

...

module Utils = {
  ...
  @live @obj
  external make_input: (
    ~id: string,
    ~inputA: inputA
  ) => input = ""
  @live @obj
  external make_inputA: (
    ~a: int,
    ~b: Js.Null.t<int>=?
    ~c: Js.Null.t<int>=?
    unit,
  ) => inputA = ""

I think usage would be a bit more ergonomic, i.e.

make_inputA(~a=1, ~b=Js.Null.return(2), ())

compared to using Js.Nullable.t:

make_inputA(~a=1, ~b=Js.Nullable.return(2), ~c=Js.Nullable.undefined)

I'm not sure how this interacts with #349. Would all of the GraphQL nullable fields in the schema assets file be modeled as Js.Nullable.t or option<Js.Null.t>? If not, I think that breaks sharing types. Maybe the directive just changes which make functions are created and the types are always modeled to allow null and undefined?

mununki commented 1 year ago

@zth I'd like to join this discussion. AFAIK, the option type for the nullable field in the mutation input is not distinguishing between null and undefined: the undefined means omitting the field when we request the mutation. So, we can't distinguish the mutation between removing and not updating for the nullable input field. I've tried to send the null value for the nullable input field as below, but it omits the field on the network request anyway.

input={a: Some(Obj.magic(Js.Nullable.null)), ...}

IMHO, it would need to get a type definition as Js.Nullable.t<'a> with optional field record syntax for the nullable field in the mutation input. Maybe we can introduce a new type for the input field:

type input = {
  a: int // required
  b?: Js.Nullable.t<int> // nullable
}

let i0 = {
  a: 1,
  b: Js.Nullable.null, // null
}
let i1 = {
  a: 1,
  // undefined
}

I guess it lets us modeling both cases of null and undefined.

mununki commented 1 year ago

FYR, https://github.com/graphql/graphql-spec/pull/83

zth commented 1 year ago

I did an implementation intended to solve this here: https://github.com/zth/rescript-relay/pull/426

@mununki @mbirkegaard what do you think?