-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Service capabilities / error behaviors #1163
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
base: main
Are you sure you want to change the base?
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 |
|
Could this be split in 2 RFCs?
Adoption could look like:
|
|
It could be split, but I would do the capabilities feature first and then onError so we don't need a period of time where you send onError and hope - better to be concrete from the beginning IMO as client capabilities may depend on it being honoured. It makes sense to ship capabilities alongside something that the spec itself needs it for in my opinion - could be error behaviors, could be fragment arguments, but I think it should be something. |
|
We have discussed the service capabilities proposal last week in the Composite Schema Working Group, and I wanted to share our perspective on this. We'd strongly advocate for having service capabilities represented in the SDL. This is essential for the schema composition tooling as only SDL exposes type system directives and the introspection format does not carry these yet. Also most tooling for composing schemas does not dynamically introspect a source schemas but rather works without needing to start a GraphQL server against schema files. Concretely, we are discussing for quite some time how we can inspect capabilities like batching support. Depending on if a server supports batching or not the schema composition would allow for features like cross-service data requirements. There are other things that we think would be great to inspect in that process like the schema name and other metadata. We did consider introducing our own capabilities directives to the specification, that would attach to the schema definition node. But to be honest this always felt like duck taping these things into GraphQL. We agree that we should keep it simple with a string-base value for service capabilities. This allows us to start using it immediately without complicating the spec too much. It also leaves room for future enhancements, such as structured metadata or enums, as the ecosystem evolves. We also think this should be decoupled from the error proposal as this would solve so many other concerns in GraphQL. |
|
Have merged the latest |
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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: