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

[martinbonnin]

This is _ErrorAction and not __ErrorAction because I get a validation error else. But ultimately, I'd vote for this to be __ErrorAction

[JoviDeCroock]

What's the validation error? Sorry can't run the code atm

[martinbonnin]

Complains that types starting with __ are reserved for builtin types. Which makes sense but since it is not standard yet... But also, it might become standard...

[benjie]

You need to add it to the introspection types I think:

graphql-js/src/type/introspection.ts

Lines 644 to 655 in eb9b6c8

	export const introspectionTypes: ReadonlyArray<GraphQLNamedType> =
	Object.freeze([
	__Schema,
	__Directive,
	__DirectiveLocation,
	__TypeNullability,
	__Type,
	__Field,
	__InputValue,
	__EnumValue,
	__TypeKind,
	]);

That's where it should be defined too.

[benjie]

It's an interesting one because it's reserved but it's not actually part of introspection. So it should either become a well known (like String, ID, Int, Boolean, Float) or it should be reserved (__) which is currently "reserved for introspection". Since it can't be populated by a variable we don't need the type name in a document so it's not super important, but also feels weird to have an __ type in a directive that we're expecting users to use heavily.

[benjie]

Maybe it should just be ErrorAction and it's a new reserved name.

[martinbonnin]

it should be reserved (__) which is currently "reserved for introspection".

TIL. I always thought of __ as reserved irrespective of introspection. Is there a specific reason why this mentions introspection explicitely here? Maybe we could reserve all types starting with a double underscore?

[martinbonnin]

Now that the directive doesn't take arguments, the problem has disappeared 😄 . Should we explicitely reserve @experimental_disableErrorPropagation somewhere? Guessing it's the same issue with @defer and @oneOf. Is there a process to "reserve" directive names?

[benjie]

No need to reserve it, it’s sufficiently complex IMO, extremely unlikely someone would implement this to do something else or to require arguments.

[martinbonnin]

Perfect 👍

[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

[JoviDeCroock]

What's the validation error? Sorry can't run the code atm

[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.