Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Query level nullability review and drafting thread #1
Query level nullability review and drafting thread #1
Changes from 45 commits
53c3d6e
ff04f2d
b823d7c
20aa350
7f12742
acf4d00
486b23f
37d78b3
438bcac
b625197
4829dfd
70762e3
1bc2272
f859034
ad40331
c9a925d
7871906
c6723ce
2e31037
c7764d5
1c20e26
e9cb076
7255a8d
254fd85
3bd4f32
96d0bee
e3b91d1
e0a572b
4aab23b
3c45346
d891d96
54a06eb
e6fc0ed
550ffeb
8515128
c60e590
e1e363a
5d5a519
6e54093
6d5d16c
458ee4e
e32b1f2
b265238
da52795
27069c9
844248c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider reducing the focus on specific client languages.
The crux of the problem is that the SDL nonNull (!) eliminates the possibility of partial failure on a given type. This forces the SDL author to decide for which fields partial failure is acceptable. A GraphQL schema author may not be in the best position to decide whether partial failure is acceptable for a given canvas. Additionally, the answer to whether a missing value is acceptable may vary between UI canvases. Specifying nonNull (aka. required) in the query gives the UI developer the power to decide where partial failure is acceptable, and the flexibility for different canvases to have different requirements in this regard.
The pains are more noticeable in languages with first-class null safety, but are relevant to any client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's one of the cases where this would be needed.
GatsbyJS projects use GraphQL to handle build-time data dependencies, and provide a file system route API that can automatically build pages from specific types.
Creating a file called
src/pages/{BlogPost.slug}.jsx
creates pages from all BlogPost nodes and injects page context.{ id: string, slug: string }
user can use the page context as an argument to fetch additional data.
This can be done with additional static analysis in plugin layer. But the proposal can be a simple solution.
Even if It's not clear in production cases that can customize schema, but it seems a huge advantage for ecosystem players using SDL and code-generation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for sharing another use case!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be true even with custom syntax. For composability of fragments, it would be worrisome if this new syntax made it impossible to have these two fragments in the same application:
With consistency, I could end up re-rendering a component built with
UserMustHaveName
based on a response fetched using onlyUserOkWithoutName
.And in fact it gets worse:
If my best friend, when I fetched
BestFriendWithName
, isUser:1
, and then suddenly, after fetchingBestFriendWithoutName
isUser:2
, I no longer am capable of rendering the component relying onBestFriendWithName
(becausename
is unset onUser:2
). If the client library doesn't know what to do in that case, you can end up with consistency updates causing crashes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you said, the client library will need to understand this new syntax/directive. I am not familiar with GraphQL client implementations, so I am not sure how difficult my imagined behavior will be to implement. Doesn't this behavior solve the issue you outlined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's how Apollo solves it, but that strategy isn't true universally. For instance that's not how Relay solves it. The client I work on can't solve it this way, as our consistency subscriptions are at the response level, for queries that include hundreds of fragments (whether that's a good choice or not is irrelevant: it's how things work for the largest GraphQL client at Facebook): if missing
name
meant the entire consistency update should not be served, whenever a product chose to mark their field with!
, they could cause entire unrelated surfaces to lose consistency.Another valid strategy would also be to return the data, but with
best_friend
being null: all the other data in the fragment/consistency subscription would still be delivered (this is Relay's strategy for@required(action: NONE)
). Or requiring the product to null check every piece of data, always, regardless of its nullability annotation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that Relay requires field selections on the same id and type to behave the same globally? If so, I can see how my suggested behavior won't work. If not:
This is what I imagined. The client would nullify up until the most immediate nullable parent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wxue Do you remember why we didn't want this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should remove this as an alternative, I don't think we understand it well enough to vouch for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defaults can be quite useful from a resilience standpoint. It is relevant to this proposal in that the
!
syntax would't be as amenable to specifying a default, whereas a directive (e.g.@nonNull(default: false)
) would open us up to that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though replacing
!
with! = “default”
like in variable defaults could work. This would either have to be evaluated client side (like in Relay) or the same default would have to apply to all references at the same location (whether from fragments or directly).