coinbase-pro-node
coinbase-pro-node copied to clipboard
bennycode
Add WebSocket channel auto-resubscription
@bennycode Here is a rough draft of #421. I've not written tests or even run the code yet. I just didn't want to go through full validation of it without getting your input on the architecture I've gone with. Could you take a look at what I have so far and give me any direction on changes you'd like to see?
In essence I've created a hashmap by channel name and keep the WebSocketChannel product IDs updated as subscribe and unsubscribe functions are called. If disconnect is called or the connection drops, the default behavior is to remember the previous subscriptions and re-subscribe to them when the connection comes back up. I added an optional parameter to the disconnect function to intentionally forget subscriptions if desired.
@bennycode I added tests and updated the code to fix some bugs I had in my original version. If you would like changes or improvements in the API of this PR, I'm happy to make the changes and re-submit.
Also, you'll notice that the code coverage is failing because of some branches missing. However, I'm not sure how to fix that because I don't understand why those lines are failing branch coverage. Could you give me some direction on that?
After looking further into the code coverage issues, it appears that these are edge cases that nyc can't identify correctly. In one case where I use hasOwnProperty, that's a known scenario where it's not accurate. In the other, the product_ids property should never be undefined because we wouldn't have gotten to that code if it was. However, we still must use the ?. operator which I think is what it doesn't like.
On a different note, I was wondering if we should add either a constructor parameter to designate whether or not we want WebSocketClient to automatically resubscribe...
constructor(baseURL: string, private readonly signRequest: (setup: RequestSetup) => Promise<SignedRequest>, autoResubscribe: boolean = true)
...or maybe add it as an option in the connect function:
connect(reconnectOptions?: Options & { autoResubscribe: boolean }): ReconnectingWebSocket
...or, have it default to resubscribe but add a function called disableAutoResubscribe() that can be called anytime during the WebSocketClient's lifecycle.
Each has merit, but I probably lean toward the disableAutoResubscribe() option. Anyway, interested in your thoughts. Sorry for being so prolific!
Hi @hjoelr. No need to be sorry! I am very happy that you put so much thought in this library. Can you exchange ?. with !.? I am infavor of adding a param to connect to decide if we should re-subscribe or not. Thus users do not have to commit themselves from the beginning and can change their re-subscription easily on a second connect.
@bennycode Okay, I made those tweaks as you suggested. I'm running up against another code coverage issue that I don't know how to solve. Any ideas why it's saying line 405 isn't covering a branch and how I can fix it?
Also, I was trying to write a test to make sure it doesn't actually resubscribe upon reconnect. However, I couldn't figure out how to do it since I'm basically needing to test something like "make sure the subscription callback doesn't run more than once". I'm not sure how to do that. Any ideas?
Is this line 405 that you want to test?
https://github.com/bennycode/coinbase-pro-node/blob/48d4dbc04754c5ed28acb95feb8fe3506703f99d/src/client/WebSocketClient.ts#L405
If yes, then the coverage tool wants you to test the positive case and the negative case for that if-condition. When you test only one of the two, then it will complain with "isn't covering a branch".
Regarding the subscription callback... Can you work with something like once? https://nodejs.org/api/events.html#emitteronceeventname-listener
@bennycode This PR is ready for final review. Please let me know if there's anything else you think I should do on it.
Can you rebase it with the "main" branch? I will try to review it then next week (I am off for the weekend).
@bennycode I've had to set this project aside for a while, which is why I've been a little silent here. Also, I've been contemplating if this change is actually beneficial. I was finding in my case that I would have to keep a separate store of requested subscriptions anyway as I want to have a way to identify which part of my program is requesting to subscribe/unsubscribe and only actually unsubscribe from a product if every part in my program that requested to subscribe also requested to unsubscribe. This means I need to store some token to identify which part of the program requested the subscribe. The PR as it stands doesn't currently support something like this.
This PR could be reworked to include such functionality, but then it makes me wonder if that's really outside the scope of this library. Anyway, I'd be interested if you have any thoughts on that.
Hi @hjoelr, unfortunately I didn't check your PR as expected. Is it still relevant? Can we move portions of it into new PRs to merge it bit-by-bit without breaking any API contract?
Closing this because of inactivity. 😶🌫️
@hjoelr just tell me if you plan to get back to it and I will take the time to support you. 💪