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

Support both WASM and GopherJS #28

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

jancona
Copy link

@jancona jancona commented Jul 5, 2019

This PR is based on @nightexcessive's wasm branch (see #27). It:

  • Replaces gopherwasm/js with Go 1.12 syscall/js
  • Uses gopherjs 1.12-2's support for syscall/js to make the WASM code work in GopherJS as well

Based on the discussions in #27, I made the main websocket package no longer depend on websocketjs. (I moved the necessary code into websocket.) websocketjs is kept unchanged for backwards compatibility, but is GopherJS-only. The end result should be that no breaking changes are made to the either package, but per @dmitshur's suggestion, the low-level websocketjs is not provided for WASM.

Let me know if you'd prefer that I make the PR against the wasm branch instead of master. I'm also happy to cleanup the commit history if you think that's appropriate.

@nhooyr
Copy link
Contributor

nhooyr commented Sep 22, 2019

@jancona I've integrated WASM compilation into nhooyr.io/websocket v1.6.0, I think you'll find it useful.

@jancona
Copy link
Author

jancona commented Sep 25, 2019

Thanks @nhooyr I'll take a look.

@jancona
Copy link
Author

jancona commented Sep 25, 2019

This PR is currently broken on Go 1.13 because of golang/go#32402. I can put together a fix, but I won't bother if there's no interest in merging this. Could someone associated with the project (@dmitshur or others) comment on whether this is worth pursuing?

@justinclift
Copy link

@johanbrandhorst What're your thoughts on this?

@johanbrandhorst
Copy link
Contributor

I think it's worthwhile getting this in but I don't have merge access either unfortunately :(.

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

5 participants