core: subscriptionExchange's error observer does not handle GraphQLError[].

[zenios]

Describe the bug

As per graphql-ws specification onError might return an array of graphql errors.

As is right now makeErrorResult reports them all as one NetworkError but most critical it does not extract a message since its expecting a single GraphQLError rather than an array

Reproduction

no-reproduction

Urql version

4.0.5

Validations

• [X] I can confirm that this is a bug report, and not a feature request, RFC, question, or discussion, for which GitHub Discussions should be used
• [X] Read the docs.
• [X] Follow our Code of Conduct

[kitten]

I can't accept this as a bug, since it's not a bug and at best a feature request.

There's really only two options here though, it's either going to be supported by @urql/core's subscriptionExchange, by graphql-ws as an option, or by Userland in the implementation — in other words, we don't "own" the integration between the two, we can only make recommendations.

Generally, @urql/core's subscriptionExchange expects a simple contract: https://github.com/urql-graphql/urql/blob/51f67adebd03dbac409e35305a44490e29bab695/packages/core/src/exchanges/subscription.ts#L78

Any observable-like that's passed to it must fulfil next events on the observer callbacks that are of the ExecutionResult format. Apart from this, error events are expected to be network errors.

This is also in part because subscription-transport-ws, which is now obviously obsolete & archived, dictated that errors on the observer are used for exceptions and not GraphQL errors: https://github.com/apollographql/subscriptions-transport-ws/blob/36f3f6f780acc1a458b768db13fd39c65e5e6518/src/client.ts#L200

I'd love for @enisdenjo to weigh in here, as this is more of a question for graphql-ws, but the general gist here is similar to GraphQL execution in general. We assume that the next event's result is sufficient to indicate that a GraphQL operation has been execution, and I'd say that accepting a list of errors from two places is a layer of unnecessary indirection for @urql/core to handle, as I'd see it as redundant 😅

No hard feelings here either, but reproductions are requested for a reason, and you aren't even linking anything. Please don't do that ♥️ You've seen the issue presumably, and I have not, which complicates things

For now, I've dug up this but it doesn't really expand on this edge case fully: https://github.com/enisdenjo/graphql-ws/blob/1fb0275cb9876280699d5144b0ac8af805834a15/src/common.ts#L75

I'm unsure when GraphQLError[] if at all would be passed to here, and if that's similar to ExecutionResult['errors'], or when this happens, but for now I'd recommend you to wrap the error handler then and pass it off to next instead.

[kitten]

I opened a PR since this is quite trivial to add, but I'll still hold off on merging it since I have no confirmation on this being actually an “active codepath” (i.e. actively used in graphql-ws).

Either way, it's probably impossible to go back to every user that's got this in production and to simply update the docs on our end. So, if this behaviour is confirmed, it kind of forces my hand

[zenios]

To be honest i was not sure if i had to fill this in urql of graphql-ws.

I don't understand the reasoning behind an array of GraphQLErrors as well.

With regards to a reproduction i find the description adequate enough to give an understanding of what is the issue. I am not in the "every issue filled needs a reproduction" camp. Sorry

The way i solved it right now was to handle this in user card an intercept the event between graphql-ws and urql sink witch then unwraps the array and return forward the first error only. I know its not the optimal but its better than an error without a message.

FYI here is the link for grahpql-ws protocol that describes the error https://github.com/enisdenjo/graphql-ws/blob/1fb0275cb9876280699d5144b0ac8af805834a15/PROTOCOL.md?plain=1#L144

[enisdenjo]

There are 2 types of errors in graphql-ws (and GraphQL in general I guess):

• operation/validation errors - errors that prevent executing an operation and cannot be recovered. GraphQL's validation can return an array of errors. These errors are therefore streamed through the error event.
• execution errors - errors that happen during operation execution, but mustn't always be terminating. For example, when subscribing, one event can contain an error while another one doesn't. These errors are a part of the GraphQL execution result and are streamed through the next event.

[kitten]

@enisdenjo I can merge the patch then. It's good to hear that — in principle — these errors generally occur pre-execution, however I'd still say I wouldn't expect any errors relating to GraphQL to come through the observer error event.

The code path seems unnecessary and conceptually speaking, the errors are the result of the execution, so, having looked at the Apollo Link for this as well, it seems that this code path just looks out of place 😅