Skip to content

Support both WASM and GopherJS#28

Open
jancona wants to merge 16 commits into
gopherjs:masterfrom
jancona:wasm-combined
Open

Support both WASM and GopherJS#28
jancona wants to merge 16 commits into
gopherjs:masterfrom
jancona:wasm-combined

Conversation

@jancona

@jancona jancona commented Jul 5, 2019

Copy link
Copy Markdown

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

nhooyr commented Sep 22, 2019

Copy link
Copy Markdown
Contributor

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

@jancona

jancona commented Sep 25, 2019

Copy link
Copy Markdown
Author

Thanks @nhooyr I'll take a look.

@jancona

jancona commented Sep 25, 2019

Copy link
Copy Markdown
Author

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
Copy Markdown

@johanbrandhorst What're your thoughts on this?

@johanbrandhorst

Copy link
Copy Markdown
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.

5 participants