Implement core API for WASM #142

nhooyr · 2019-09-21T18:21:11Z

Closes #121

nhooyr · 2019-09-21T19:59:45Z

@albrow is it possible you could review this?

Closes #121

nhooyr · 2019-09-22T01:12:39Z

@coadler pls review if you can

coadler

Looks good

coadler · 2019-09-22T01:22:35Z

internal/wsecho/wsecho.go

+	ctx, cancel := context.WithTimeout(ctx, time.Minute)
+	defer cancel()
+
+	b := make([]byte, 32768)


Since you used 1 << 40 above 32 << 10 would be consistent here

Also just realized 1 << 40 is a terabyte, not a gigabyte lol.

albrow · 2019-09-22T01:29:28Z

Sure. Should be able to review tomorrow.

nhooyr · 2019-09-22T07:11:06Z

ci/wasm.sh

+GOOS=js GOARCH=wasm go test -exec=wasmbrowsertest ./... -args "$WS_ECHO_SERVER_URL"
+
+kill %1
+wait -n || true


Will improve this test to assert proper closure of the connection.

nhooyr · 2019-09-22T17:23:16Z

Going to merge for now since I want to restructure the codebase but please review if you can @albrow. I'll fix anything you point out afterwards.

albrow

@nhooyr looks pretty good overall. I just left some comments about build tags and not blocking inside of js.FuncOf.

Is there any plan to implement io.ReadWriteCloser on top of this API? that would be really helpful for a variety of use cases.

albrow · 2019-09-23T18:57:38Z

README.md

 - First class [context.Context](https://blog.golang.org/context) support
 - Thorough tests, fully passes the [autobahn-testsuite](https://github.com/crossbario/autobahn-testsuite)
 - [Zero dependencies](https://godoc.org/nhooyr.io/websocket?imports)
 - JSON and ProtoBuf helpers in the [wsjson](https://godoc.org/nhooyr.io/websocket/wsjson) and [wspb](https://godoc.org/nhooyr.io/websocket/wspb) subpackages
 - Highly optimized by default
 - Concurrent writes out of the box
+- [Complete WASM](https://godoc.org/nhooyr.io/websocket#hdr-WASM) support


nit: This is an abbreviation (not an acronym) so it's usually spelled "Wasm".

albrow · 2019-09-23T19:12:09Z

internal/wsjs/wsjs.go

-func (c *WebSocket) addEventListener(eventType string, fn func(e js.Value)) {
-	c.v.Call("addEventListener", eventType, js.FuncOf(func(this js.Value, args []js.Value) interface{} {
+func (c WebSocket) addEventListener(eventType string, fn func(e js.Value)) func() {
+	f := js.FuncOf(func(this js.Value, args []js.Value) interface{} {
 		fn(args[0])


It's really important to not block inside of a js.Func wrapped function. Doing so will block the JavaScript event loop and prevent any other JavaScript functions from running. You should almost always start a new goroutine inside of js.FuncOf.

For example:

f := js.FuncOf(func(this js.Value, args []js.Value) interface{} { go fn(args[0]) return nil })

Nice catch, fixed.

albrow · 2019-09-23T19:13:45Z

internal/wsjs/wsjs.go

-func (c *WebSocket) SendText(v string) (err error) {
+// SendText sends the given string as a text message
+// on the WebSocket.
+func (c WebSocket) SendText(v string) (err error) {
 	defer handleJSError(&err, nil)
 	c.v.Call("send", v)


Do we have good test coverage for the SendText method? I've had a lot of trouble with things like this in the past because JavaScript does wonky and unexpected things with text encoding. The string you are trying to send might not end up being the same as what is received on the other end. It might even be better to just remove this method.

We have good test coverage here. See https://github.com/nhooyr/websocket/blob/ef5a88d1828ab3da40b07b9faacf627b0421b329/websocket_js_test.go#L39

@nhooyr it looks like randString only includes hex characters? You should probably test with more unicode characters, in particular ones with higher code points. There can be differences in how these are encoded/interpreted in Go and JavaScript.

That's fair. I'll adjust it.

Fixed in #148

albrow · 2019-09-23T19:19:53Z

websocket_js.go

@@ -0,0 +1,217 @@
+package websocket // import "nhooyr.io/websocket"


Shouldn't this file have the build tag: +build js,wasm?

Files with _js are automatically compiled only for GOOS=js.

👌 Combined with the other thread about not needing wasm in the build tags, this should be fine.

albrow · 2019-09-23T19:38:28Z

wsjson/wsjson_js.go

@@ -0,0 +1,58 @@
+// +build js


Unless you know that a file is compatible with GopherJS you usually want to use the build tag +build js,wasm. The addition of wasm clarifies for the Go compiler that this file is compatible with WebAssembly but tells the GopherJS compiler that the file is not compatible with GopherJS. This blog post explains the situation in a little more detail.

Basically it works like this:

Build tag Vanilla Go WebAssembly GopherJS

(none) ✅ ✅ ✅

js ❌ ✅ ✅

!js ✅ ❌ ❌

js,wasm ❌ ✅ ❌

js,!wasm ❌ ❌ ✅

The library should just work with GopherJS once gopherjs/gopherjs#929 is closed.

It won't even compile on GopherJS versions that don't support Go 1.13 because we use the new wrapping functions in the errors package.

reason being syscall/js is shimmed by GopherJS so that it works the same as Go Wasm.

Gotcha. Seems fine if you are not supporting versions older than 1.13.

nhooyr · 2019-09-23T20:19:14Z

Is there any plan to implement io.ReadWriteCloser on top of this API? that would be really helpful for a variety of use cases.

Already implemented. websocket.NetConn is compiled into Wasm builds.

nhooyr · 2019-09-23T20:24:02Z

In this PR it wasn't but I made it work in follow up PR #146

nhooyr force-pushed the wasm branch 2 times, most recently from bc1defe to b6caac6 Compare September 21, 2019 19:10

nhooyr marked this pull request as ready for review September 21, 2019 19:57

scsmithr approved these changes Sep 21, 2019

View reviewed changes

nhooyr force-pushed the wasm branch from b6caac6 to 631999c Compare September 21, 2019 23:06

nhooyr changed the title ~~Add WebSocket API~~ Implement core API for WASM Sep 21, 2019

Implement core API for WASM

599534b

Closes #121

nhooyr force-pushed the wasm branch 2 times, most recently from dc34ee5 to cf3e427 Compare September 22, 2019 00:01

Add WASM test

ff4d818

nhooyr force-pushed the wasm branch 5 times, most recently from 1b7cc02 to 52ff3bf Compare September 22, 2019 01:12

nhooyr force-pushed the wasm branch 4 times, most recently from fe9ba71 to e301872 Compare September 22, 2019 01:27

coadler approved these changes Sep 22, 2019

View reviewed changes

nhooyr force-pushed the wasm branch 7 times, most recently from ab3489c to 06aa2f7 Compare September 22, 2019 02:23

nhooyr force-pushed the wasm branch 4 times, most recently from 6d131b8 to a9618cb Compare September 22, 2019 02:47

Add WASM and GopherJS docs

26f0793

nhooyr force-pushed the wasm branch from a9618cb to 26f0793 Compare September 22, 2019 02:47

nhooyr mentioned this pull request Sep 22, 2019

Add WebAssembly support (GOARCH=wasm). gopherjs/websocket#25

Open

Update line count in the README

5da52be

nhooyr mentioned this pull request Sep 22, 2019

net.Conn is the wrong API for WebSockets gopherjs/websocket#29

Closed

nhooyr commented Sep 22, 2019

View reviewed changes

Improve wasm test coverage

76a6a26

nhooyr force-pushed the wasm branch from 572300a to 76a6a26 Compare September 22, 2019 17:22

nhooyr merged commit 7b4bd30 into master Sep 22, 2019

nhooyr deleted the wasm branch September 22, 2019 17:24

albrow reviewed Sep 23, 2019

View reviewed changes

nhooyr mentioned this pull request Sep 23, 2019

Fixes from @albrow's Wasm review #148

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement core API for WASM #142

Implement core API for WASM #142

nhooyr commented Sep 21, 2019

nhooyr commented Sep 21, 2019

nhooyr commented Sep 22, 2019

coadler left a comment

coadler Sep 22, 2019

nhooyr Sep 22, 2019

albrow commented Sep 22, 2019

nhooyr Sep 22, 2019

nhooyr commented Sep 22, 2019

albrow left a comment

albrow Sep 23, 2019

nhooyr Sep 23, 2019

albrow Sep 23, 2019

nhooyr Sep 23, 2019

albrow Sep 23, 2019

nhooyr Sep 23, 2019

albrow Sep 23, 2019 •

edited

Loading

nhooyr Sep 23, 2019

nhooyr Sep 23, 2019

albrow Sep 23, 2019

nhooyr Sep 23, 2019

albrow Sep 23, 2019

albrow Sep 23, 2019

nhooyr Sep 23, 2019

nhooyr Sep 23, 2019

nhooyr Sep 23, 2019

albrow Sep 23, 2019

nhooyr commented Sep 23, 2019 •

edited

Loading

nhooyr commented Sep 23, 2019

		@@ -0,0 +1,217 @@
		package websocket // import "nhooyr.io/websocket"

Build tag	Vanilla Go	WebAssembly	GopherJS
(none)	✅	✅	✅
`js`	❌	✅	✅
`!js`	✅	❌	❌
`js,wasm`	❌	✅	❌
`js,!wasm`	❌	❌	✅

Implement core API for WASM #142

Implement core API for WASM #142

Conversation

nhooyr commented Sep 21, 2019

nhooyr commented Sep 21, 2019

nhooyr commented Sep 22, 2019

coadler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albrow commented Sep 22, 2019

Choose a reason for hiding this comment

nhooyr commented Sep 22, 2019

albrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albrow Sep 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhooyr commented Sep 23, 2019 • edited Loading

nhooyr commented Sep 23, 2019

albrow Sep 23, 2019 •

edited

Loading

nhooyr commented Sep 23, 2019 •

edited

Loading