Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(websocket): switch to nhooyr.io/websocket for wasm support #290

Merged
merged 4 commits into from Jul 29, 2021

Conversation

Milerius
Copy link
Contributor

@Milerius Milerius commented Jul 28, 2021

What is achievied:

I do not that have that much experience with golang & websocket, i tested my implementation, it works for me but I don't know if it was the most idiomatic way to do it, feel free to comment and give your opinion so that we can move in the right direction.

It is especially the first time that I have to use a context and I have a little doubt about the lifespan of the context in this specific case, I tried not to break the architecture in place (2 chans quit / done) but suddenly I don't know if on the lifetime side the cancel function of the context will still be available at the time of the websocket closure.

cc @adshao

Address #284

Motivation for this pr: I am currently writing a trading bot that also works in webassembly, having the websocket binance would allow me to have the prices in real time rather than having to do polling in my browser

- KeepAlive heartbeat implemented using nhooyr/websocket#265 (comment)
- Only spot websocket API switched to nhooyr websocket atm
- Close the websocket using StatusNormalClosure
@adshao adshao closed this Jul 28, 2021
@adshao adshao reopened this Jul 28, 2021
@adshao
Copy link
Owner

adshao commented Jul 28, 2021

Hi @Milerius , thanks for this PR. It looks good to me, I will build and test it in my environment to check if everything works.

adshao
adshao previously approved these changes Jul 29, 2021
@adshao
Copy link
Owner

adshao commented Jul 29, 2021

Hi @Milerius , I have tested it, it seems great. Could you please refactor for delivery and futures packages as well?

@Milerius
Copy link
Contributor Author

Hi @Milerius , I have tested it, it seems great. Could you please refactor for delivery and futures packages as well?

Sure i will do shortly.

@Milerius
Copy link
Contributor Author

Hi @Milerius , I have tested it, it seems great. Could you please refactor for delivery and futures packages as well?

done @adshao

@adshao adshao merged commit 86f8bfb into adshao:master Jul 29, 2021
adshao added a commit that referenced this pull request Mar 15, 2022
due to issue: #321,
and nhooyr.io/websocket is not active for maintain.

revert pr #290.

Signed-off-by: adshao <tjusgj@gmail.com>
adshao added a commit that referenced this pull request Mar 15, 2022
due to issue: #321,
and nhooyr.io/websocket is not active for maintain.

revert pr #290.

Signed-off-by: adshao <tjusgj@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants