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

Compile for WebAssembly #121

Closed
nhooyr opened this issue Aug 27, 2019 · 10 comments · Fixed by #142
Closed

Compile for WebAssembly #121

nhooyr opened this issue Aug 27, 2019 · 10 comments · Fixed by #142
Assignees

Comments

@nhooyr
Copy link
Contributor

nhooyr commented Aug 27, 2019

@albrow I saw you in issue gorilla/websocket#432 (comment)

If you'd like to contribute the necessary improvements to this library, I'd be more than happy to help, review and merge your changes.

@albrow
Copy link

albrow commented Aug 27, 2019

@nhooyr the API for nhooyr/websocket is much smaller than gorilla/websocket and more closely resembles the JavaScript API. It looks doable.

My main motivation for a WebAssembly-compatible WebSockets library is to support running libp2p/go-ws-transport in the browser. I have already made that possible with libp2p/go-ws-transport#51. It's hard to justify spending time on a different approach when we already found one that works.

That said, we could potentially reduce the maintenance burden for libp2p/go-ws-transport by changing it to use this package and abstracting away the Wasm/JavaScript related code. I don't know how open the libp2p team would be to that idea. If we were to go this route, another important thing to consider is browser compatibility and backwards compatibility for anyone still using gorilla/websocket.

Have you done any testing to see if this package is compatible with browsers and/or gorilla/websocket?

@nhooyr
Copy link
Contributor Author

nhooyr commented Aug 27, 2019

That said, we could potentially reduce the maintenance burden for libp2p/go-ws-transport by changing it to use this package and abstracting away the Wasm/JavaScript related code. I don't know how open the libp2p team would be to that idea.

That would be fantastic.

backwards compatibility for anyone still using gorilla/websocket.

This would be unfortunate but I don't see a good way around it. The libraries are fundamentally different.

I meant in terms of API, but yes they both can talk to each other just fine.

Have you done any testing to see if this package is compatible with browsers and/or gorilla/websocket?

Yes, I've done extensive research and experimentation. You can see it all in the issue history. I've read gorilla/websocket's source and issue tracker nearly completely.

We use this library at https://coder.com, it fully passes the autobahn test suite (which tests compatibility between WebSocket implementations), has a solid array of tests and 35 or so unique cloners a day (400 clones usually) so it is used in the wild.

@nhooyr
Copy link
Contributor Author

nhooyr commented Aug 27, 2019

Oh my bad, I misread your response. You're asking whether it's compatible with the browser API.

Definitely only a subset would work but I'll take a closer look tonight and get back to you.

@albrow
Copy link

albrow commented Aug 27, 2019

@nhooyr Sorry I wasn't super clear. I'm actually asking about connection compatibility. Can you dial from a browser to a server that is using nhooyr/websocket? (It sounds like the answer is yes). Can you dial from nhooyr/websocket to gorilla/websocket and vice versa?

@nhooyr
Copy link
Contributor Author

nhooyr commented Aug 27, 2019

Yea, will work fine.

@nhooyr nhooyr pinned this issue Aug 27, 2019
@nhooyr
Copy link
Contributor Author

nhooyr commented Aug 28, 2019

The API's that will need to be removed when compiling for WASM:

  • Accept as you cannot accept WebSocket's on browsers (unless WASM also works on nodejs and you can accept there?)
  • DialOptions's HTTPClient and HTTPHeader
  • The streaming APIs
  • SetReadLimit
  • Ping
  • CloseRead
  • Export then unexpected status codes (they're unexpected because they don't make sense for Go)

It'd be an extremely stripped down API.

@nhooyr
Copy link
Contributor Author

nhooyr commented Aug 31, 2019

I'm not sure if this will be worth it given those considerations. It'd require refactoring the code quite awkwardly wherever the WASM splits are required and involve quite a bit of complexity. I wonder if it'd be better to have a sub package /wasm that implements a subset of the API for WASM.

@nhooyr nhooyr added the complex label Sep 1, 2019
@nhooyr
Copy link
Contributor Author

nhooyr commented Sep 20, 2019

So I'm convinced this is a good idea and shouldn't be too hard this weekend.

@nhooyr nhooyr removed the complex label Sep 20, 2019
@nhooyr nhooyr self-assigned this Sep 20, 2019
@nhooyr
Copy link
Contributor Author

nhooyr commented Sep 21, 2019

Will ask https://github.com/gopherjs/websocket to be deprecated once this is done.

@nhooyr
Copy link
Contributor Author

nhooyr commented Sep 21, 2019

Related: gopherjs/websocket#28

nhooyr added a commit that referenced this issue Sep 21, 2019
nhooyr added a commit that referenced this issue Sep 21, 2019
nhooyr added a commit that referenced this issue Sep 21, 2019
nhooyr added a commit that referenced this issue Sep 21, 2019
nhooyr added a commit that referenced this issue Sep 21, 2019
@nhooyr nhooyr unpinned this issue Sep 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants