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

Heartbeat #76

Merged
merged 11 commits into from
Sep 19, 2017
Merged

Heartbeat #76

merged 11 commits into from
Sep 19, 2017

Conversation

amejiarosario
Copy link
Contributor

@amejiarosario amejiarosario commented Jul 7, 2017

Adds the heartbeat option to Websocket

@amejiarosario
Copy link
Contributor Author

amejiarosario commented Jul 7, 2017

Could any of you please review these changes?
@awitherow @gcperrin @givanse @mihar Thanks!

Copy link
Contributor

@awitherow awitherow left a comment

Choose a reason for hiding this comment

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

Looks good to me!

I just would like to see some better error handling and a more verbose readme of how the websocket function works for people who might not be confident enough to dig into the source code.

var self = this;
options = options || {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Due to the breaking changes in the way the WebsocketClient takes parameters, do you think it would be good to add the following more parameter error handling?

For example, maybe checking to ensure that options is in fact an object, and not a string (perhaps websocketURI).

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the only case either, just a thought of one potential.

The idea is to look at this and ensure it is as backwards compatible friendly as possible, without making it mess.

README.md Outdated

Optionally set the heartbeat mode.
```javascript
var websocket = new Gdax.WebsocketClient(['BTC-USD','ETH-USD'], {heartbeat: true});
Copy link
Contributor

Choose a reason for hiding this comment

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

Here it would be good to expand upon the optional parameters (websocket URI, authentication) just so that people know more without having to dig into the code themselves to figure out how it works, or why their old implementation is breaking if they've upgraded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I'll update

@fb55
Copy link
Contributor

fb55 commented Aug 25, 2017

@amejiarosario This needs a rebase, great patch otherwise.

@amejiarosario
Copy link
Contributor Author

@awitherow made it backward compatible.
@fb55 rebased!

@fb55 fb55 merged commit 86c2987 into coinbase:master Sep 19, 2017
@fb55
Copy link
Contributor

fb55 commented Sep 19, 2017

@amejiarosario Thanks a lot! We should update this to use the heartbeat channel, but I didn't want to block this any further.

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

3 participants