Add support for @experimental_disableErrorPropagation

[martinbonnin]

This pull request adds support for @onError(action: NULL) to disable error propagation for error aware clients:

This pull request adds support for @experimental_disableErrorPropagation to disable error propagation for error aware clients:

"""
Disables error propagation.
"""
directive @experimental_disableErrorPropagation on QUERY | MUTATION | SUBSCRIPTION

I'm not super used to write TypeScript, feel free to amend the PR as needed but I figured it'd be good to have.

The logic is unconditional. The matching graphql-java PR has a specific opt-in flag so that it's not enabled by accident in the very unlikely event that a schema already contains a matching directive. Let me know if this is an issue.

Many thanks @JoviDeCroock for pointing me in the right direction 🙏

See graphql/nullability-wg#85
See graphql/graphql-spec#1050

[github-actions]

Hi @martinbonnin, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

• @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
• @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

[JoviDeCroock]

Looks good to me, a few formatting things but those would be nitpicks. Is this good to go from a WG standpoint CC @twof @benjie

[benjie]

We've not really debated what this should be yet, I'd propose going with an argument-less and obviously experimental directive such as @experimental_disableErrorPropagation. We may also want the directive to have an impact on introspection (for example returning semantic non-null types rather than strictly non-null types for output fields), so the proposals may or may not be tied together - using an explicitly experimental directive will allow people to play with the execution behavior without committing us to the result shape of introspection/etc.

[martinbonnin]

@benjie I renamed to @experimental_disableErrorPropagation. I like that it's very explicit 👍 . It's also very long but in my case, the directive is going to be added automatically by the Apollo Kotlin compiler so it's perfectly fine.

[benjie]

I've written up a counter proposal because I think that error propagation should be disabled on a per-client basis rather than a per-operation basis; but I have no issues with this being merged so that people can start experimenting with disabling null propagation - that's the entire point of having experimental directives people can use!

Counter-proposal is to make this a request parameter rather than an operation directive: graphql/nullability-wg#86

[martinbonnin]

@benjie agree a request parameter would be better. Modifying the document is always taking the risk that what the user types in GraphiQL doesn't match what the client is ultimately sending over the wire. I think we have the same issue with fragment arguments.

One nice thing with the directive though is that feature discovery works out of the box. Whereas if we use a request parameter, we'd have to introduce a change in the response or something like this to indicate support. Which feels like a bigger lift.

All in all, it's probably not the end of the road but I would still like this to be merged so that we can experiment in parallel of discussing other alternatives.

[benjie]

One nice thing with the directive though is that feature discovery works out of the box. Whereas if we use a request parameter, we'd have to introduce a change in the response or something like this to indicate support. Which feels like a bigger lift.

One nice thing about the request approach is you don't need feature discovery - you simply turn it on if you're an "error handling client". If the server doesn't support it your request should still succeed and you'll just have some selection sets be blown up as it would have been anyway, so you're no worse off than you were.

[martinbonnin]

Alright, if we agree that we don't need anything in the response, it might be worth going this road directly.

My only objection then is that it potentially requires pull requests in all the servers out there, right (apollo, yoga, etc... )? Because those are the ones calling execute() in the first place? So it'll take yet another round of review/discussions before end users can actually try it?

Whereas the directive solution is "just" a matter of "update your graphql-js dependency and you can start using the new directive"?

If that's the case, then I'd stick with the long, explicitely experimental directive for now. We can always ask everyone to update all their libs later, once we have a better view of the end game.

[benjie]

I'm not arguing against using the directive for now, I think we should do that. But long term, I think the request parameter is better.

[martinbonnin]

Sounds good 👍 . I'll wait for @JoviDeCroock's input and if we all agree, I'll update the graphql-java PR to match

[JoviDeCroock]

The request parameter would take the shape of a RequestExtension I presume or do we have other opinions on that?

I am personally a bigger fan of letting language features be described by the Document rather than by aligning some request parameter to have a say over the execution mode of operations contained within the Document.

With removing the arguments from the directive we are setting ourselves up for poor scalability of the directive, are we open to doing this? I guess we can always add optional arguments but just wanted to surface this. If all of these concerns are good then let's move forward

[martinbonnin]

The request parameter would take the shape of a RequestExtension I presume or do we have other opinions on that?

I think @benjie had a top-level key in mind?

https://github.com/graphql/graphql-js/pull/4192/files#diff-1d705c6a4c73cd3ce46190029e75abd3015b49de3ec250357dcf9b44a35cb7d0R162

I am personally a bigger fan of letting language features be described by the Document rather than by aligning some request parameter to have a say over the execution mode of operations contained within the Document.

No strong opinion here besides the fact that we don't have directives on document (yet)?

With removing the arguments from the directive we are setting ourselves up for poor scalability of the directive, are we open to doing this?

I'm fine with this. As the long experimental name implies, it's clearly not the final form of this so we can always change later.

Also, we could decide to go with @abortOnError and a validation rule if we wanted to add it in addition to the current @disableErrorPropagation. Not as nice IMO but another option.

[benjie]

The request parameter would take the shape of a RequestExtension I presume or do we have other opinions on that?

No; the GraphQL Foundation should never put anything in extensions; the point of extensions is to be a reserved namespace for third parties to put what they want in it - if we start putting stuff there, it no longer serves that purpose. It would be a top-level key like query, variables, operationName, extensions.

I am personally a bigger fan of letting language features be described by the Document rather than by aligning some request parameter to have a say over the execution mode of operations contained within the Document.

You should probably give that feedback against my proposal directly; however do think about what this means for users - users who shouldn't need to know how their client works will now need to remember to include this directive in every document (or have tooling add it for them), and in 10 years time, the vast majority of GraphQL documents should have this directive, making it effectively a "use strict" style of annotation. I'd argue, as I have before, that this is actually a client concern. The client is explicitly stating "I'd like to do the error handling on the client side, no need for you to do it for me on the server side" - the consumer (the person writing the GraphQL document) doesn't actually care which way it's done in most cases.

With removing the arguments from the directive we are setting ourselves up for poor scalability of the directive, are we open to doing this?

We could add arguments in future with a non-breaking change so long as either they're nullable or they have defaults? But also, it's explicitly @experimental_* so this shouldn't be a big deal anyway.

[JoviDeCroock]

You should probably give that feedback against my proposal directly; however do think about what this means for user

I'm going to digest my thoughts more before I do but generally I am really opposed to little option flags on a top-level payload