Skip to content
This repository has been archived by the owner on May 5, 2020. It is now read-only.

Websocket 1.1.0 proposal #19

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

bgourlie
Copy link

@bgourlie bgourlie commented Feb 13, 2017

Proposed changes

Add onOpen and onClose subscriptions

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

@process-bot
Copy link

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.

@edevil
Copy link

edevil commented Feb 13, 2017

The hero we needed. I very much approve these changes and I hope you get more feedback.

@bmurphy1976
Copy link

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! :)

@bgourlie
Copy link
Author

bgourlie commented Feb 18, 2017

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:

  • After discussing with people on slack and elsewhere, it seems quite a few people rely on this behavior
  • Making it optional will add complexity to the implementation
  • With onOpen and onClose subscriptions, it's possible to track the connection state and prevent messages from being sent at the application level
  • This would no longer require a major version bump as all changes would be backwards compatible.

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

@bgourlie bgourlie changed the title Websocket 2.0.0 proposal Websocket 1.1.0 proposal Feb 18, 2017
@cdevienne
Copy link

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)

Namek added a commit to Namek/artemis-odb-entity-tracker that referenced this pull request Jul 14, 2017
based on elm-lang/websocket#19 - merged with binary data support
Copy link

@ibizaman ibizaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that can be of any help, I just have one comment about this PR. It LGTM although I don't have enough experience for that to be worth something.

What's the next step for this, can I help in some way? Maybe by doing some tests on my side?

@@ -1,5 +1,5 @@
{
"version": "1.0.2",
"version": "2.0.0",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should become 1.1.0 I suppose

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ibizaman the public function keepAlive no longer exists, making this a breaking change for consumers. Hence a major version bump is mandatory.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is a proposal, and while it's a breaking change as-is, if something were to be decided on where this PR could be cleaned up for actual merging, the intent would be to make it non-breaking.

@antarestrader
Copy link

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add subscriptions for connection opened and connection closed.
8 participants