Use undefined to mean not loaded, use null to mean gql null

[zenflow]

This fixes #101 and implements the use of maybe vs maybeNull that I suggested in #51 (comment):

@brobertsUPS @elie222 It seems to make sense to use both maybe & maybeNull:

• maybe, so value can be undefined when value hasn't been loaded from graphql server yet
• maybeNull only for fields that are nullable (i.e. not explicitly non-null) according to the graphql schema, so value can be null when the value loaded from graphql server is null.

Right now the field nullability according to the graphql schema is ignored (assumed to be nullable), and all fields (except id) can be null (which could either mean that it's actually null or that it's not loaded yet).

So the breaking changes of this enhancement:

1. All fields that used types.maybeNull and defaulted to null before now use types.maybe and default to undefined
2. All array/list fields now use types.maybe instead of types.optional and default to undefined instead of []
3. The nullability of a field according to the graphql schema is now respected and represented with types.maybeNull (meaning some fields will use both maybe & maybeNull)

There are 2 1 exceptions to # 3 to make life easier:

1. The identifier field is non-nullable regardless of what the graphql schema says (as before)
2. List elements are non-nullable regardless of what the graphql schema says. Lists themselves are still nullable depending on what the graphql schema says. E.g. In graphql [Thing]! is a non-nullable list of nullable Things. I don't think we ever intentionally make list elements nullable, only by accident (tell me if you disagree), so it's a nice convenience to just always treat list elements as non-nullable. (edit: Withdrawn. See Use undefined to mean not loaded, use null to mean gql null #102 (comment))

@chrisdrackett @mattiasewers If you agree with the principle of the change then I will finish this WIP by updating the rest of the examples and tweaking the README.

[mtsewrs]

@chrisdrackett can you take a look? Am not that familiar with the generator, it looks ok to me though

[elie222]

So I agree it’s nice to assume every item in a list is non null, but a reason a developer May choose to allow these items to be nullable is because of how GraphQL works with non null fields that return null. For example I may have post.date set to be non null, but for some reason null is returned for date. What happens is that this makes the entire post object null. If we also have the list set to have non null items this would mean no items are returned by the server rather than just the single problematic post. This is a problem in general with graphql. The more you use non nullable, the less data that will be returned to the client. If everything is non nullable then nothing will be returned even on the smallest of errors for a single field deep in the tree. I personally would respect a user that decides to make a list item nullable. Is there a reason to not be consistent with the user’s schema?

…

On Wed, 25 Sep 2019 at 12:50 Mattias Ewers ***@***.***> wrote: @chrisdrackett <https://github.com/chrisdrackett> can you take a look? Am not that familiar with the generator, it looks ok to me though — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#102?email_source=notifications&email_token=AAXSQX76AUDARPBYOJ2ORRTQLMX73A5CNFSM4I2GSD72YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7RJZUI#issuecomment-534944977>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXSQX7MN42P2WTKAH74ZFTQLMX73ANCNFSM4I2GSD7Q> .

[k-ode]

@zenflow I was just going to create an issue about this.

My thinking is that we wouldn't use maybeNull at all. All fields gets wrapped in types.maybe, and nullable fields are a union of the type and null.

nonNullableStringProp: types.maybe(types.string);
nullableProp: types.maybe(types.union(types.null, types.string))

So omitting a property means it defaults to undefined, while null is something that you have to pass in yourself.

[zenflow]

@elie222 Interesting! Thanks for the explanation of this gotcha with using non-null in graphql!

I personally would respect a user that decides to make a list item
nullable. Is there a reason to not be consistent with the user’s schema?

No, there's no valid reason to ignore this part of the user's schema, now that I understand why we would want nullable list elements.

I was in part motivated by the fact that we already force the identifier field to non-null. This is also not being consistent with the user's schema for purpose of convenience, so it's a similar scenario. But I think this is acceptable because we require that the field be non-null.

I will update the PR so that the nullability of list items/elements is respected and can be null or non-null.

[zenflow]

@kimgronqvist types.maybeNull(string) is just sugar for types.union(types.null, types.string), so it sounds like we have the exact same idea for how to handle null & undefined!

[k-ode]

types.maybeNull(types.string) is sugar for types.optional(types.union(type, types.literal(null)), null)

Here's some code to show the difference:

const MyType = types.model({
  nullableProp: types.maybe(types.union(types.string, types.null))
});
const t = MyType.create();

console.log(t.nullableProp); -> undefined

const MyTypeMaybeNull = types.model({
  nullableProp: types.maybe(types.maybeNull(types.string))
});
const tMaybeNull = MyTypeMaybeNull.create();

console.log(tMaybeNull.nullableProp); -> null

[zenflow]

@kimgronqvist Yup, my statement was a little inaccurate.

And after thinking about it, I updated the PR to use types.union(types.undefined, types.null, types.string) instead of types.maybeNull(types.maybe(types.string)) since the former is more straight-forward and readable (especially for those unfamiliar with mst types) and has a more intuitive default value (the natural default value for when something hasn't been defined: undefined). See commit: "Fixup 4d40053, improve readability of output code" (9174dcf).

@kimgronqvist thanks for the helpful suggestion/suggestiveness

[k-ode]

@zenflow Thanks for working on this!

But it needs to be optional, right? Otherwise the model will throw when given incomplete data.

[zenflow]

But it needs to be optional, right? Otherwise the model will throw when given incomplete data.

@kimgronqvist No, there's no real need to use types.optional because when given incomplete data, the fields that are not defined are (by natural default) undefined, which is a valid value already. The model will accept incomplete data without errors.

We could wrap with types.optional(..., undefined) to explicitly declare the default as undefined. Personally I would prefer to leave it an implicit default for the sake of brevity. Does anyone prefer the verbose & explicit form?

[zenflow]

I will update the PR so that the nullability of list items/elements is respected and can be null or non-null

@elie222 This is done in commit 718f6d7 "Fixup 4d40053, respect nullability of list items in gql schema"

[zenflow]

Removed the "WIP" status from this PR since I'm happy with it and consider it ready for review.

[zenflow]

It's ready for review, but examples 1, 3 & 4 still need to be updated before merging

[zenflow]

Published this on npm as @zen_flow/mst-gql version 0.6.0-canary.1 if anyone wants to test it in their project.

[chrisdrackett]

@zenflow I can take a look next week!

[chrisdrackett]

@zenflow looking at this now!

[chrisdrackett]

for something like

mst-gql/examples/1-getting-started/src/app/models/TodoModel.ts

Line 23 in ad46826

self.complete = !self.complete

what would you suggest? I know this isn't directly related to this issue, but I got to thinking about it while reviewing.

my gut is generally to avoid toggles and have a setComplete and setIncomplete that would be used in the component based on the current state.

Seems like with boolean values it would be good practice to check for null before evaluating the boolean.

[chrisdrackett]

@zenflow This looks good to me! I only looked over the examples and didn't try it in anything more complicated, but I don't currently foresee any issues. I was going to update the examples, but this PR isn't from the main mst-gql repo. I think once those are updated we should be good to merge!

[zenflow]

@chrisdrackett Yeah, I don't like that toggle() depends on a field that might not be loaded. I think probably one setComplete(complete: boolean) method would be better than setComplete() and setIncomplete() though.

Another thing you could maybe do in the future, if you really wanted a toggle() method (no arguments) is use model.ensureLoaded(...fieldNames), e.g.

  async toggle () {
    await self.ensureLoaded('complete')
    const complete = !self.complete
    return self.store.mutateSetTodoComplete({ id: self.id, complete }, undefined, () => {
      self.complete = complete
    })
  }

[zenflow]

Soon I'll be playing around with this model.ensureLoaded method in my app codebase and I'll probably make a PR to add it here at some point. I will keep a equal function signature with model.hasLoaded, meaning I will make hasLoaded accept multiple field names. (All of them must be loaded to return true)

The awesome thing is it would be able to, on a model-instance-by-model-instance basis, only request the fields that aren't already loaded. Rather than behaving that way always, we probably would want to use a fetchPolicy. All of the fetch policies used for queries (except 'no-cache' which is kinda defunct and needs to be removed) seem to apply here!

The main difficulty of it is that I want it to handle not just multiple field names (e.g. todo.ensureLoaded('text', 'complete', 'assignee')) but also query selectors (e.g. todo.ensureLoaded(todo => todo.text.complete.assignee(user => user.name)))

[chrisdrackett]

nice! I'm going to add this to the changelog and do a 0.6.0 release.