Conversation
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
I have applied the RFC1 label to this as inherited from the PR it replaces: |
|
A full, CI-passing, implementation of this is available in GraphQL.js now; check it out: |
GraphQLConf requires speakers to be familiar with "inclusive naming", and the inclusive naming guides encourage the avoidance of the word "abort" where possible: https://inclusivenaming.org/word-lists/tier-1/abort/
|
This PR description is currently out of date; I've reworked the capabilities infrastructure based on Lee's feedback, see the changes for details. |
|
@benjie @leebyron given the previous discussions on feature discovery have been going on for years, I'm doubtful we can land |
|
I have:
I believe this is ready for another look. @michaelstaib What do the composite schemas WG think about this? |
|
In case service capabilities gets quite big, I wonder if we should have things like extend type __Service {
capabilities(
"""
If present, filters the returned capabilities to only those whose
canonical identifiers appear in the list.
"""
identifiers: [String!]
): [__Capability!]!
}query Probe {
__service {
capabilities(identifiers: [
"graphql.fragmentArguments",
"graphql.onError",
]) { name value }
}
} |
| QualifiedName :: | ||
|
|
||
| - QualifiedName . Name [lookahead != `.`] | ||
| - Name . Name [lookahead != `.`] |
There was a problem hiding this comment.
Is this expecting to grammatically require that at least one dot appears, or should Name on its own should be a qualified Name as well, and we just use validation to ensure it's the shape that we expect?
That might help build IDE tooling, if the first Name is a valid grammar, so you get better typeahead completion.
There was a problem hiding this comment.
Requiring 2+ names was intentional; we could do it with a validation rule but the "qualified" in "qualified name" is intended to indicate that at least one period is required.
|
I really like this proposal and already am imagining all the things it'll simplify. Does it make more sense to re-use our existing type system even more? What I'm thinking is that Service is a root type and you can define sub-capabilities underneath it. |
OK after having experimented a little bit with how we’d use it, re-using root type syntax is actually not a good idea. The service capabilities need arguments with constant values, and do NOT need a type like fields do, so re-using existing syntax would make it less clear what’s happening. I’m a fan of the proposal as-is! |
|
@benjie any thoughts about breaking this in 2 different proposals?
I can see a bunch of cases where teams could start using Thoughts? Edit: I've just noticed #1208 👍 , let's do the same for |
| - {"NULL"}: The _response position_ must be set to {null}, even if such position | ||
| is indicated by the schema to be non-nullable. (The client is responsible for | ||
| interpreting this {null} in conjunction with the {"errors"} list to | ||
| distinguish error results from intentional {null} values.) |
There was a problem hiding this comment.
In the case of mutation resolution, does "NULL" mean execution continues?
eg if I have a mutation:
mutation {
doThing1
doThing2
doThing3
}and all fields were not nullable, which of these responses would be correct in the presence of an error on resolution of doThing2:
A: response fields deviate from graphql request ask
{
"data": {
"doThing1": true,
"doThing2": null
/* doThing3 never resolved */
},
"errors": [
{ /* error from doThing2 */ }
]
}B: synthetic field production - propagate null to down-operation resolution
{
"data": {
"doThing1": true,
"doThing2": null,
"doThing3": null /* never resolved but server injects null because field is requested */
},
"errors": [
{ /* error path doThing2 */ }
]
}C: continue resolving
{
"data": {
"doThing1": true,
"doThing2": null,
"doThing3": null /* server application's responsibility for stopping execution after error */
},
"errors": [
{ /* error path doThing2 */ },
{ /* error path doThing3 thrown due to response errored flag set, does not indicate a problem with doThing3 */ }
]
}or perhaps more confusingly
{
"data": {
"doThing1": true,
"doThing2": null,
"doThing3": true
},
"errors": [
{ /* error from doThing2 */ }
]
}There was a problem hiding this comment.
The final “perhaps more confusingly” is the correct behavior according to the current semantics. It’s an interesting question, because if the client changes the onError without the knowledge of the user, the resulting side-effects will differ. Option C is interesting, but also wrong in its own way. In my own schemas, root level fields (even mutations) are nullable so it wouldn’t make a difference to me, I wonder if making the fields non-nullable is done specifically to block follow-up mutations currently…? Though it does mean you wouldn’t see the result of previously completed mutations so it seems like a weird choice.
There was a problem hiding this comment.
Hot Chocolate's mutation convention makes mutations not nullable and it is quite annoying to change: https://chillicream.com/docs/hotchocolate/v16/building-a-schema/mutations/#mutation-conventions and I think semantically it is correct (the mutation always has a result or there is an error).
I agree the current wording suggests option C as well (both of the last 2 samples are option C). The difference between them is if the server implementer (or framework they use) flag that an error occurred via some mechanism such that if inside ExecuteMutation() the flag is set, every following ExecuteSelectionSet() immediately raises an error.
I suppose my point is that either application authors (both clients and servers) will need to beware of of the possibility that this could happen (and frameworks could take an opinionated stance or provide a default) or the spec force A or B and clients (and potentially client libraries) would need to be able to handle such responses. I don't think there is a satisfying answer here.
There was a problem hiding this comment.
@bbarry can you ellaborate why C. is not satisfying?
onError: NULL makes it possible to resolve more data, whether for queries or subscriptions. Forcing A. or B. would be very surprising to me. With C. the server can decide how it wants to handle things. If anything, we could recommend to have only a single root field in mutations. I think this was discussed at some point but can't find it anymore.
Edit: found it! It's "batched mutations" article: https://medium.com/@xuorig/graphql-mutation-design-batch-updates-ca2452f92833
This is a rewrite of
It is re-implemented on top of the recent editorial work (e.g. renaming
_field error_to_execution error_) and also makes a significant change in that it does not requireonErrorto be included in the response, instead an introspection field is used to indicate:onErroris supportedonErroris not presentReplaces:
GraphQL.js implementation:
Please see this 60 second video on the motivation for this PR (the last few seconds of the video also covers "transitional non-null" which is a separate concern).
As agreed at the nullability working group, disabling error propagation is the future of error handling in GraphQL. Error propagation causes a number of issues, but chief among them are:
Clients such as Relay do not want error propagation to be a thing.
This has traditionally resulted in schema design best practices advising using nullable in positions where errors were expected, even if
nullwas never a semantically valid value for that position. And since errors can happen everywhere, this has lead to an explosion of nullability and significant pain on the client side with developers having to do seemingly unnecessary null checks in loads of positions, or worse - unsafely bypassing the type safety.The reason that GraphQL does error propagation is to keep it's "not null" promise in the event that an error occurs (and is replaced with
nulldue to the way GraphQL responses are structured and limitations in JSON) in a non-nullable position.It doesn't take much code on the client to prevent the client reading a
nullthat relates to an error, graphql-toe can be used with almost any JavaScript or TypeScript based GraphQL client (not Relay, but it has@throwOnFieldErrorthat you can use instead) and achieves this in 512 bytes gzipped - and that's with a focus on performance rather than bundle size.This PR allows the client to take responsibility for error handling by specifying
onError: "NULL"as part of the GraphQL request, and thereby turns off error propagation behavior. This is also set as the recommended default for future schemas.With clients responsible for error handling, we no longer need to factor the possibility of whether something can error or not into its nullability, meaning we can use the not-null
!to indicate all the positions in the schema for whichnullis not a semantically valid value - i.e. the underlying resource will never be a legitimatenull.The end result:
<ErrorBoundary />I've also included
onError: "HALT"in this proposal. We've discovered that there's a small but significant class of clients out there, mostly ad-hoc scripts, that throw away the entire response when any error occurs. By codifying this into the spec we make it easier to implement these clients, and we allow the server to halt processing the rest of the request unnecessarily.As noted by @revangen in this comment: