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

misc/wasm: "this" missing in wasm callbacks #27441

Closed
theclapp opened this issue Sep 1, 2018 · 6 comments
Closed

misc/wasm: "this" missing in wasm callbacks #27441

theclapp opened this issue Sep 1, 2018 · 6 comments

Comments

@theclapp
Copy link
Contributor

@theclapp theclapp commented Sep 1, 2018

What version of Go are you using (go version)?

go version go1.11 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env:
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/lmc/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/lmc"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/32/8nd008752lv7v3vsgzlrw6qr0000gn/T/go-build736262546=/tmp/go-build -gno-record-gcc-switches -fno-common"

Edited to add GOARCH=wasm GOOS=js go env:

GOARCH="wasm"
GOBIN=""
GOCACHE="/Users/lmc/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="js"
GOPATH="/Users/lmc/src/goget:/Users/lmc/src/go_appengine/gopath"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="0"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/32/8nd008752lv7v3vsgzlrw6qr0000gn/T/go-build048369190=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Wrote a Go/wasm callback and wanted to access this from the callback.

What did you expect to see?

Some way to access this.

In Javascript, if you call some_object.some_method(), then inside some_method, a special variable called this will be set to some_object. If you call some_other_object.some_method(), then this will be set to some_other_object.

In Go/wasm, the js.NewCallback() function is the only way to get a function that Javascript can call. (Your callback is not called directly, but pushed onto a list of callbacks to be run by a separate goroutine.) js.NewCallback arranges for your function to get any function arguments passed to the actual callback, but there's no way to get to the value of this.

What did you see instead?

There's no way to access this.

Every Javascript function has a this, even if it's undefined.

I think either NewCallback should have an explicit this argument:

func NewCallback(fn func(this Value, args []Value)) Callback

or put this as args[0] for every callback.

Discussion

I believe this issue is independent of (a)synchronous callbacks (#26045). The value of this could be captured when the "real" callback actually runs, saved, and passed in to the Go code when it runs, whether it's synchronous or asynchronous.

Sample code:

diff --git a/misc/wasm/wasm_exec.js b/misc/wasm/wasm_exec.js
index 94b9552c59..94036c2685 100644
--- a/misc/wasm/wasm_exec.js
+++ b/misc/wasm/wasm_exec.js
@@ -390,7 +390,7 @@

                static _makeCallbackHelper(id, pendingCallbacks, go) {
                        return function() {
-                           pendingCallbacks.push({ id: id, args: arguments });
+                         pendingCallbacks.push({ id: id, args: arguments, this: this });
                                go._resolveCallbackPromise();
                        };
                }
diff --git a/src/syscall/js/callback.go b/src/syscall/js/callback.go
index 9d573074cb..ded9cde03c 100644
--- a/src/syscall/js/callback.go
+++ b/src/syscall/js/callback.go
@@ -109,9 +109,10 @@ func callbackLoop() {
                        }

                        argsObj := cb.Get("args")
-                   args := make([]Value, argsObj.Length())
-                   for i := range args {
-                           args[i] = argsObj.Index(i)
+                 args := make([]Value, argsObj.Length()+1)
+                 args[0] = cb.Get("this")
+                 for i := range args[1:] {
+                         args[i+1] = argsObj.Index(i)
                        }
                        f(args)
                }
@ALTree ALTree added the Arch-Wasm label Sep 1, 2018
@ALTree ALTree changed the title "this" missing in wasm callbacks misc/wasm: "this" missing in wasm callbacks Sep 1, 2018
@ALTree ALTree added this to the Go1.12 milestone Sep 1, 2018
@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Sep 2, 2018

I have encountered a need for this only in DOM event callbacks to access the original element. A common way to achieve this is to use the NewEventCallback, and do ev.Get("target") to get the source element.

What is the scenario where you need to access the original object which is other than the source element in an event callback ?

@theclapp

This comment has been minimized.

Copy link
Contributor Author

@theclapp theclapp commented Sep 2, 2018

@ALTree Thanks for cleaning up this issue.

@agnivade I'm writing a wasm wrapper for Vue.js. (Actually converting one from GopherJS.) Vue.js frequently passes what it calls "the current vm / Vue instance" as this in various handlers (see for example here, "All lifecycle hooks are called with their this context pointing to the Vue instance invoking it"), and I see no other way to get that piece of information.

Even if there were such a way (in my particular context of writing a Vue.js wrapper), in other contexts it's easy to imagine that that would not be the case. Not having access to this is analogous to only being able to write regular Go methods like func (_ T) funcName(...), i.e. with no access to the receiver.

I understand that one way or the other adding this will change the API and that it should not go in sooner than 1.12, and probably concurrently with or as a part of #26045, I just didn't feel that that issue sufficiently called out the need for this.

@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Sep 2, 2018

Understood. Thanks for clarifying.

/cc @neelance

@myitcv

This comment has been minimized.

Copy link
Member

@myitcv myitcv commented Sep 3, 2018

@theclapp and I caught up offline on this.

Whilst not explicitly called out, #26045 does silently imply the addition of the this parameter: @neelance and I discussed as much offline.

So this issue might be useful in case it's worth adding this parameter support independent of #26045, but as @theclapp and I discussed offline, there are issues with doing so (this might not be in the state that the callback handler expects, much like the event/other arguments)

@theclapp

This comment has been minimized.

Copy link
Contributor Author

@theclapp theclapp commented Sep 5, 2018

For folks following along at home: I've had good luck with a JS factory function that captures this and calls the Go function with it explicitly:

// Javascript factory function
function call_with_this(f) {
	return function() {
		f(this, ...arguments);
	}
}
// Go factory function; wraps js.NewCallback
func NewCallback(f func(this js.Value, args []js.Value) interface{}) js.Value {
	return js.Global().Call("call_with_this",
		js.NewCallback(func(args []js.Value) {
			f(args[0], args[1:])
		}))
}
neelance added a commit to neelance/go that referenced this issue Oct 14, 2018
With this change, callbacks returned by syscall/js.NewCallback
get executed synchronously. This is necessary for the APIs of
many JavaScript libraries.

Fixes golang#26045
Fixes golang#27441

Change-Id: I591b9e85ab851cef0c746c18eba95fb02ea9e85b
neelance added a commit to neelance/go that referenced this issue Oct 14, 2018
With this change, callbacks returned by syscall/js.NewCallback
get executed synchronously. This is necessary for the APIs of
many JavaScript libraries.

Fixes golang#26045
Fixes golang#27441

Change-Id: I591b9e85ab851cef0c746c18eba95fb02ea9e85b
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 14, 2018

Change https://golang.org/cl/142004 mentions this issue: all: add support for synchronous callbacks to js/wasm

neelance added a commit to neelance/go that referenced this issue Oct 14, 2018
With this change, callbacks returned by syscall/js.NewCallback
get executed synchronously. This is necessary for the APIs of
many JavaScript libraries.

Synchronous calls to callbacks that have been triggered by calls from
Go to JavaScript get executed on the same goroutine. Asynchronous
callbacks get executed on an extra goroutine.

Fixes golang#26045
Fixes golang#27441

Change-Id: I591b9e85ab851cef0c746c18eba95fb02ea9e85b
@gopherbot gopherbot closed this in 6dd70fc Nov 10, 2018
@golang golang locked and limited conversation to collaborators Nov 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.