Open bgourlie opened 7 years ago
Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!
Here is what to expect next, and if anyone wants to comment, keep these things in mind.
The hero we needed. I very much approve these changes and I hope you get more feedback.
Good stuff. We have a dashboard that traces logs in real-time and can connect to individual servers to trace running processes using websockets. You have to be on the VPN in order for this functionality to work. Having these additions would allow us to make the user experience much better when the user is not connected to the VPN (i.e. actually tell them they might need to connect). This is long overdue! :)
Quick update:
I've updated the proposal (not the pull request) to not change message queuing/save behavior. I did this for a couple reasons:
onOpen
and onClose
subscriptions, it's possible to track the connection state and prevent messages from being sent at the application levelI did add another item to the proposal, and that is resetting the reconnect backoff when the page becomes visible. This would make reconnects immediate when the user opens the tab, instead of having to potentially wait minutes for the reconnect to happen.
I think disabling queuing can be done without a major version bump, by adding additional constructor. We would have :
type MySub msg
= Listen String Bool (String -> msg)
| KeepAlive String
listen : String -> (String -> msg) -> Sub msg
listen url tagger =
subscription True (Listen url tagger)
listenNoQueuing : String -> (String -> msg) -> Sub msg
listenNoQueuing url tagger =
subscription False (Listen url tagger)
First, I would like to express my support for this patch.
I would then like to request an additional feature. I would like the details
from the onClose
method in line 346 of the proposed patch to be sent with Die
and be added to the onClose
subscription.
Proposed changes
Add
onOpen
andonClose
subscriptionsThis would fix #14 and #10 and was my primary motivation for proposing these changes. These subscriptions are important for pretty much any non-trivial scenario, and appears to be the most requested feature for the websocket package.
Reset backoff when page becomes visible
With exponential backoff fixed, reconnect attempts occur far less frequently than when it was broken. It makes sense to reset the backoff when the page becomes visible, otherwise, the user would have to refresh the page if they want the application to reconnect immediately.
Feedback
This pull request is the result of my hacking over the weekend and there are probably issues or things that need to be cleaned up. I also removed
keepAlive
temporarily just to make it easier to hack in the proposed changes. I'm interested in hearing feedback for the proposed changes, and if/when something concrete is decided, I will clean up this PR.