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

Option to opt-out from re-conecting to WebSocket #989

Closed
fassko opened this issue Feb 3, 2020 · 5 comments
Closed

Option to opt-out from re-conecting to WebSocket #989

fassko opened this issue Feb 3, 2020 · 5 comments

Comments

@fassko
Copy link
Contributor

fassko commented Feb 3, 2020

Let me explain why this would be needed.

GraphQL subscriptions connect to the server and all is good, but if it looses connection (network issue or re-deploy of the server) currently WebSocketTransport tries to reconnect. When connection it tries to write to send to the queue but if not acked it saves message to the queue. If this happens multiple times the result is multiple of the same subscription message in the queue.

Next step would be to keep better WebSocket state using IDs from subscription message not just incremented IDs, that would help to avoid duplicate websocekt message instances in the WebSocketTransport.queue.

if this sounds like a feature to add I can come up with a PR.

@designatednerd
Copy link
Contributor

It seems reasonable - my understanding of the websocket stuff is slightly hand-wavy, but this seems like a reasonable thing to allow opting out of, since the default behavior of trying to reconnect is probably what most people want, but your situation of not wanting it also makes sense.

Would love to see a PR!

@fassko
Copy link
Contributor Author

fassko commented Feb 3, 2020

@designatednerd I can help out with other stuff with Websockets stuff as I have dealt with that a lot and I help to maintain the Starscream library and wrote some articles about the topic. :)

@designatednerd
Copy link
Contributor

God that'd be a huge help. I'll ping you off GH and we can chat.

@designatednerd
Copy link
Contributor

This issue was at least partially addressed by #1004 which shipped with 0.22.0.

@fassko are there still more pieces you want to fix on this before I close it out?

@designatednerd designatednerd added this to the 0.22.0 milestone Feb 20, 2020
@fassko
Copy link
Contributor Author

fassko commented Feb 20, 2020

Thanks, for updating Starscream I created #1030 you can assign it to me.

@fassko fassko closed this as completed Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants