-
-
Notifications
You must be signed in to change notification settings - Fork 81
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
Use undefined to mean not loaded, use null to mean gql null #102
Conversation
@chrisdrackett can you take a look? Am not that familiar with the generator, it looks ok to me though |
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>
.
|
@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.
So omitting a property means it defaults to undefined, while null is something that you have to pass in yourself. |
@elie222 Interesting! Thanks for the explanation of this gotcha with using non-null in graphql!
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. |
@kimgronqvist |
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 |
@kimgronqvist Yup, my statement was a little inaccurate. And after thinking about it, I updated the PR to use @kimgronqvist thanks for the helpful suggestion/suggestiveness |
@zenflow Thanks for working on this! 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 We could wrap with |
Removed the "WIP" status from this PR since I'm happy with it and consider it ready for review. |
It's ready for review, but examples 1, 3 & 4 still need to be updated before merging |
Published this on npm as |
@zenflow I can take a look next week! |
@zenflow looking at this now! |
for something like
my gut is generally to avoid toggles and have a Seems like with boolean values it would be good practice to check for null before evaluating the boolean. |
@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! |
@chrisdrackett Yeah, I don't like that Another thing you could maybe do in the future, if you really wanted a async toggle () {
await self.ensureLoaded('complete')
const complete = !self.complete
return self.store.mutateSetTodoComplete({ id: self.id, complete }, undefined, () => {
self.complete = complete
})
} |
Soon I'll be playing around with this 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 The main difficulty of it is that I want it to handle not just multiple field names (e.g. |
nice! I'm going to add this to the changelog and do a |
This fixes #101 and implements the use of maybe vs maybeNull that I suggested in #51 (comment):
So the breaking changes of this enhancement:
types.maybeNull
and defaulted tonull
before now usetypes.maybe
and default toundefined
types.maybe
instead oftypes.optional
and default toundefined
instead of[]
types.maybeNull
(meaning some fields will use bothmaybe
&maybeNull
)There are
21 exceptions to # 3 to make life easier: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(edit: Withdrawn. See Use undefined to mean not loaded, use null to mean gql null #102 (comment))[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.@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.