Skip to content
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

Add connection_error to protocol #112

Closed
andyrichardson opened this issue Feb 12, 2021 · 9 comments
Closed

Add connection_error to protocol #112

andyrichardson opened this issue Feb 12, 2021 · 9 comments
Labels
question Further information about the library is requested

Comments

@andyrichardson
Copy link

Story

Currently there is no way to send an error if a semantically invalid message is sent on connection_init.

It would be great if the protocol supported a connection_error message similar to this.

Note: I'm aware of the existing error message - it requires a subscription ID which would not be present at connection_init time

Acceptance criteria

  • The protocol accommodates for errors on connection_init
  • These errors do not have an id attribute
@andyrichardson andyrichardson changed the title Add support for error on connection_init Add connection_error to protocol Feb 12, 2021
@michelepra
Copy link
Contributor

michelepra commented Feb 16, 2021

what about #75 and #78 ???

@andyrichardson
Copy link
Author

I was looking through the server side code and it looks like the current implementation closes on failure following connection_init - I'll go with that 👍

@enisdenjo
Copy link
Owner

Exactly as @michelepra suggested. The WebSocket close event is meant to communicate the connection close reason. You're free to pick any user-land close code number within 4000-4999 and whatever close reason string you like (or even a stringified JSON if you want to transfer more meaning). Use the client-side error or on.disconnect to act accordingly.

@lovasoa
Copy link

lovasoa commented Sep 8, 2023

Hasura GraphQL uses a connection_error message: #499

@enisdenjo
Copy link
Owner

This connection_error message type is OK in the deprecated subscriptions-transport-ws, but does not exist in the GraphQL over WebSocket spec and should not be used.

Also read #112 (comment).

@lovasoa
Copy link

lovasoa commented Sep 8, 2023

But is is actually used in real applications that are deployed out there... Maybe it should be added to the PROTOCOL.md in this repo ?

@enisdenjo
Copy link
Owner

Mainly because deprecated subscriptions-transport-ws is still out there.

@lovasoa
Copy link

lovasoa commented Sep 8, 2023

I'm struggling to see any upside to not adding it to PROTOCOL.md, but I clearly see the downside: people connecting to existing applications not being able to debug connection errors. Maybe the upside is avoiding to encourage new implementers to use it instead of the connection close reason ? But then, the spec could just say that. The spec in this repo is not of much use if it does not describe what's actually out there.

@enisdenjo
Copy link
Owner

As explained in #112 (comment), the connection_error message is superfluous and therefore completely unnecessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information about the library is requested
Projects
None yet
Development

No branches or pull requests

4 participants