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

Fix heartbeat channel subscriptions #150

Merged
merged 2 commits into from
Dec 26, 2017

Conversation

rmm5t
Copy link
Contributor

@rmm5t rmm5t commented Nov 30, 2017

  • Automatically adds the heartbeat channel if not already subscribed
  • Removes redundant keepalive ping on the socket

Fixes #113

this.heartbeat = heartbeat;
this.channels = channels || ['full'];
if (!this.channels.includes('heartbeat')) {
this.channels.push('heartbeat');
Copy link
Contributor

Choose a reason for hiding this comment

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

Heartbeats are pretty noisy, so there should be a way to disable them to save bandwidth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heartbeats are pretty noisy...

🤔 Not compared the full or ticker or matches or level2 channel. The only channel that is less noisy than heartbeat is the user channel.

heartbeat is mild compared to anything else, and it comes with other very useful benefits (keeps the connection open, helps address lost sequences, etc).

Also, the longer-term intent of this automatic heartbeat addition is to eventually allow the WebsocketClient to automatically monitor the heartbeat, reconnect as necessary, and potentially fire an event when a missed sequence is recognized. i.e. The heartbeat does not need to necessarily propagate to the caller.

(I'll also rebase after the new conflicts.)

@fb55
Copy link
Contributor

fb55 commented Dec 24, 2017

I like the change, just one comment

@rmm5t
Copy link
Contributor Author

rmm5t commented Dec 24, 2017

Rebased. Let me know if the automatic heartbeat channel is a show stopper. I'm in favor of keeping the automatic heartbeat. Without it, I suspect we'll need to add the keepalive socket ping back in (i.e all the deleted code from this pull-request and all the extra liability of 100+ lines of code that comes with it).

- Automatically adds the `heartbeat` channel if not already subscribed
- Removes redundant `keepalive` ping on the socket

Fixes coinbase#113
@rmm5t
Copy link
Contributor Author

rmm5t commented Dec 25, 2017

Rebased again to resolve new conflicts.

@fb55 fb55 merged commit 94c671c into coinbase:master Dec 26, 2017
@fb55
Copy link
Contributor

fb55 commented Dec 26, 2017

Thanks a lot @rmm5t!

@rmm5t rmm5t deleted the 113-fix-heartbeat-channel branch December 28, 2017 00:29
@rmm5t rmm5t mentioned this pull request Dec 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants