Implement onError proposal #4364
Conversation
|
Hi @benjie, I'm @github-actions bot happy to help you with this PR 👋 Supported commandsPlease post this commands in separate comments and only one per comment:
|
This comment has been minimized.
This comment has been minimized.
@benjie The latest changes of this PR are available on NPM as Also you can depend on latest version built from this PR: |
|
I want to add an introspection field indicating the default error behavior of a schema, so I've moved this to draft. |
|
Regarding the introspection, would we have a non-repeatable schema-directive then that indicates what kind of error-propagation we are dealing with? I think that would make sense, the server can internally dictate what the default is and it can be externally requested to be different by means of the request-parameter. I think we can merge these separately though, the default for all would be PROPAGATE at the moment so there's no need yet for the schema-directive. The name |
|
Exactly that, yes. We can do them separate, but I'd like to make it so that the presence of the field in introspection also implies the support for the parameter and vice versa. |
|
This is now up to date with the latest version of the spec RFC, including the removal of |
This comment has been minimized.
This comment has been minimized.
@benjie The latest changes of this PR are available on NPM as Also you can depend on latest version built from this PR: |
We should remove |
Never mind, I didn't notice this PR is against PS: strong preference for this PR rather than the other one |
This reverts commit 726b4ba.
| let defaultErrorBehavior: Maybe<GraphQLErrorBehavior> = schemaDef | ||
| ? getDefaultErrorBehavior(schemaDef) | ||
| : null; | ||
| for (const extensionNode of schemaExtensions) { |
There was a problem hiding this comment.
Can an extension override a previous extension or the original schema definition with respect to an aspect of behavior?
There was a problem hiding this comment.
Hmmm. No; it shouldn't. Good catch. So essentially we need "default" to be its own value (null is an option), then store a value when set, and don't allow setting it a second time. That or don't allow it on the extend schema
But also this is kind of irrelevant because this implementation is out of date vs the latest spec proposal; we'll be doing it via service { capabilities { ... } } now instead (actually... what would an extend service look like?) and that should cause a conflict if the same key is set more than once.
There was a problem hiding this comment.
@benjie what do you think of removing the introspection bits + default value from this PR? Just have the request onError? We can add the default value later on. Baby steps.
There was a problem hiding this comment.
The client needs to know if the server supports onError or not; the server should ignore onError if it doesn't understand it, so they kind of go hand in hand: the signalling that the feature is supported and the feature itself. Further, we should tie the indication of the server's default error behavior to the concept of error behaviors, so that clients and servers support both together, without that the migration is a lot more complex. This has been discussed before at the WGs and conclusion is we shouldn't ship onError without solving how to tell a client that it's supported, and out-of-band communications is not ideal for this.
Spec PR:
Allow clients to disable error propagation via request parameter graphql-spec#1153