Shopify / graphql-design-tutorial

MIT License
2.43k stars 193 forks source link

Defaulting to semantic nullability #17

Open flux627 opened 4 years ago

flux627 commented 4 years ago

This guide has been a breath of fresh air with regard to nullability handing, in stark contrast from the official GraphQL standpoint on the matter:

https://graphql.org/learn/best-practices/#nullability https://medium.com/@calebmer/when-to-use-graphql-non-null-fields-4059337f6fc8

And countless articles like this one, in agreement: https://medium.com/expedia-group-tech/nullability-in-graphql-b8d06fbd8a3c

But I prefer the practice of catching errors at the resolver level, and either allowing the query to fail, or replacing the affected fields with "blank" or error-informing ("This comment failed to load") data. For me, this is far more manageable than dealing with non-semantic nulls everywhere.

Would it make sense for this tutorial talk about this, since it is a deviation from the GraphQL standard?

eapache commented 4 years ago

I have lots of thoughts on this, but none of them are confident - while I still agree with you in theory, we've run into so many problems with non-nulls in practice that I don't feel comfortable recommending their use. The official recommendations have a bunch of trade-offs, but they're likely the right choice for the majority of use cases.

flux627 commented 4 years ago

There seems to be some dissonance here- this sentiment is not reflected in the tutorial. Would it make sense, then, to update the tutorial? Looks like @swalkinshaw is the author to the semantic null bits.

@eapache, I would love to hear your non-confident thoughts on the matter. Can you share what specific problems in practice you've run into?

eapache commented 4 years ago

Would it make sense, then, to update the tutorial?

Our thoughts on this are evolving quite a bit, and we prefer not to change the tutorial too drastically on a regular basis. We'll definitely update it if we come to a confident conclusion.

I would love to hear your non-confident thoughts on the matter. Can you share what specific problems in practice you've run into?

Given a field which our internal data model declares must always be present, you’d think it would be a safe bet to make the relevant GraphQL field non-null. We’ve collected a significant number of exceptions to this rule:

In a mostly-nullable schema, any of these problems would be isolated to the single failing field which would return null, permitting the rest of the query to succeed. This is a pretty important property for overall system resiliency.