-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Allow clients to disable error propagation via request parameter #1153
base: runtime-errors
Are you sure you want to change the base?
Conversation
should be {null}. | ||
is made {null}. | ||
|
||
**NO_PROPAGATE** |
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 kind of liked NULL
here. Probably won't die on this hill though.
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 felt like that too, but I changed it from NULL
for the following reasons:
- on error we already use
null
. It's where the null goes that's the concern, andNULL
isn't very descriptive - you have to deduce it fromPROPAGATE
being the other option onError: "NULL"
andonError: null
would be easy to mistake for beginners, but the latter would be ignored and behave the same asPROPAGATE
.
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.
Those are valid points but in the tradeoff, I would favor the simplicity, clarity and pronouncability of onError: NULL
. Negations always make me itchy.
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 hear you. How about onError: LOCAL
, that’s one I’ve considered before. I think NULL would be a mistake. Others: errorHandling: PROPAGATE|CLIENT|ABORT
; errorBoundary: SOURCE|NULLABLE|ROOT
.
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 quite like error boundary actually.
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.
errorNulls: ALLOW | NULLABLE_ONLY | FORBID
I particularly like that this dissuades NULLABLE_ONLY
(i.e. propagate) because it's longer to type
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.
nullOnError: ERROR_POSITION | PROPAGATE_TO_NULLABLE | ENTIRE_RESPONSE
or, less verbose:
nullOnError: SOURCE | NULLABLE | ROOT
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.
If we're back to having "null" somewhere, I like the initial onError: NULL | PROPAGATE | ABORT
the best.
Second choice would be onError: NO_PROPAGATE | PROPAGATE | ABORT
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 NULL would be a mistake, which is a shame because it’s nice and short. I think it will lead to a lot of issues being filed that turn out to be people doing null
rather than "NULL"
across the entire ecosystem.
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.
It's always a tradeoff of course and I have no crystal ball but I expect most people to not even have to type "NULL"
. Iit will be handled by their clients most of the time and/or by GraphiQL probably with a dropdown or something like this.
Plus the servers should definitely return a clear error if an unknown value is sent to them, making it easy to recover from any potential mistake.
And on the plus side, onError: NULL
is easy to pronounce and remember. Looking at this differently, I think it's going to be read and discussed a lot more that it's going to be written so I would optimize for that.
PS: Looking forward to everyone downvoting this comment in 2030 when onError: NULL
is the default 😄
From Sept 18 2023, @captbaritone expresses it well:
|
Replaces:
Requires editorial to be merged first:
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
null
was 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
null
due 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
null
that relates to an error, graphql-toe can be used with almost any JavaScript or TypeScript based GraphQL client (not Relay, but it has@throwOnFieldError
that 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: "NO_PROPAGATE"
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 whichnull
is not a semantically valid value - i.e. the underlying resource will never be a legitimatenull
.The end result:
<ErrorBoundary />
errors: "NO_PROPAGATE"
)I've also included
onError: "ABORT"
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 abort processing the rest of the request unnecessarily.