v2: Create with support for both GopherJS and GOOS=js GOARCH=wasm. about go-js-dom

[cryptix]

The question is how much of API differences can we tolerate under the same import path before feeling clunky.

I fear none. I assume @shurcooL was aiming for equal api so that it won’t be a breaking change, just an additional implementation.

[dmitshur]

@yml Thanks for looking at this and sharing your work. I think that's a great step forward.

The approach I'd like us to use for dom is going to be slightly different than the one you've used, but it should quite easy to adapt your code.

My proposed approach is for us to have separate files, one with // +build js,!wasm build tag (for GopherJS) and another with // +build js,wasm build tag (for Wasm). The current dom.go will remain unmodified, and the new dom_wasm.go will be very similar to the modified dom.go file in your branch, except it can use syscall/js directly.

That way, the dom API (and implementation) will be unmodified for GopherJS users and won't break any code. For example, the following change is not compatible with existing code, so we can't proceed with making it:

@@ -131,74 +131,76 @@ type Event interface {
 	PreventDefault()
 	StopImmediatePropagation()
 	StopPropagation()
-	Underlying() *js.Object
+	Underlying() js.Value
 }

We'll need to maintain and keep in sync two files rather than one, but I think that's the only viable way to proceed. Luckily, this package is mostly complete and doesn't change much, so it's not a very high cost.

If that makes sense to you, and if you'd like, feel free to make a PR with that approach. Otherwise, I or someone else can get around to it later.

From the yml#1 description, you said you only tested that it compiles. One of the additional steps we'll need to do, before we can merge a PR into this package, is test that it functions as expected.

[yml]

@shurcooL thank you very much for your feedbacks. PR #59 implements the changes you have described above recommended.

[dmitshur]

I have an update on this.

@hajimehoshi has added support for the syscall/js API to GopherJS 1.12-2 in PR gopherjs/gopherjs#908. That change makes it possible for a syscall/js-based implementation of the dom package to compile and work successfully in Go WebAssembly and GopherJS. That in turn means we no longer need to have two separate .go files, one with js,!wasm and another with js,wasm build constraints.

When we last attempted this in PR #59, we discovered a blocking problem that a significant breaking API change will be necessary, and so we'll need to use a different import path per the semantic import versioning convention, to avoid breaking existing programs importing the current v1 API.

I've taken the previous work by @yml in PR #59 and implemented the following changes on top of it:

1. merged the GopherJS and WebAssembly .go files into one (i.e., kept the syscall/js implementation only)
2. moved the code into a v2 directory so that its import path will be honnef.co/go/js/dom/v2

• I chose to go with the major subdirectory approach, because it's compatible with both GOPATH mode and module mode (while major branch approach works only with module mode), and we want to continue to support GOPATH mode for now

3. converted all struct fields with js:"foo" tags to equivalent methods; a change that was necessary because the syscall/js API does not support those js:"foo" tags
4. made js.Value embedded in BasicNode (instead of being not embedded and called Object)

• I was able to work around the name collision by using the Underlying() method in a few places where the underlying Value was needed but was shadowed by a Value() string method

5. fixed the implementation of methods that work with callbacks to use the js.Func types rather than func(*js.Object) as before

In my testing, the new v2 API is fully functional with Go WebAssembly and GopherJS. I have not found any major problems. I'll send a PR for it now (crediting @yml as a co-author).

[yml]

I have attempted to do this in this branch
I would happily change anything if it can help to achieve the goal in this ticket.

[dmitshur]

@dominikh, in PR #59, you said:

Well, I'm definitely not a fan of the massive amount of code duplication. That is to say, I'd very much like to avoid it.

(I wanted to move the discussion here, so the PR discussion can stay focused on the approach described in the PR description.)

I would like to avoid it too, but out of all alternatives, it seems the least bad. I'm happy to hear any suggestions that are better. I'm aware of the following alternatives (that do not seem better):

• Implementing gopherjs/gopherjs#833. Not seeing movement there. If it happens in future, the duplication can be easily removed.
• Changing the dom API. As is, it has methods that return *github.com/gopherjs/gopherjs/js.Object type, which is not compatible with Wasm. This would break existing applications.

Basically, with this solution, we are creating 2 separate dom packages at the same import path with the same public API. One is the current GopherJS implementation (unmodified), and the second is the very similar Wasm one. I think that's better than having 2 separate dom packages with the same public API at different import paths.

[dmitshur]

I think I've found a potentially show-stopping problem with implementing this issue. This package relies very heavily on the js struct field tag feature of GopherJS to access properties of many objects. E.g.:

go-js-dom/dom.go

Lines 1802 to 1811 in 6da835b

	type HTMLAnchorElement struct {
	*BasicHTMLElement
	*URLUtils
	HrefLang string `js:"hreflang"`
	Media string `js:"media"`
	TabIndex int `js:"tabIndex"`
	Target string `js:"target"`
	Text string `js:"text"`
	Type string `js:"type"`
	}

Wasm has no support for this feature (the only way to access fields is via the Get method of js.Value), and as far as I know, it's not planned (please correct me if I'm wrong @neelance).

That means the current dom API is not going to be possible to implement for Wasm fully. I don't have ideas for how to deal with this yet.

[yml]

I guess the alternative is to change these fields to be a method with the same name. These is yet another difference in the API with the gopherjs equivalent.

The question is how much of API differences can we tolerate under the same import path before feeling clunky.

[dominikh]

I fear none. I assume @shurcooL was aiming for equal api so that it won’t be a breaking change, just an additional implementation.

ACK. If the APIs are different, they should be different packages, i.e. different import paths.

[dmitshur]

Agreed.

@dominikh How do you feel about a dom v2 API at the import path honnef.co/go/js/dom/v2 (package name dom)?

The API would be the same as v1, except current js fields would be replaced by methods. The Underlying() *js.Object stuff will need to be discussed/figured out.

[dominikh]

Ugh. Fine.

[myitcv]

How do you feel about a dom v2 API at the import path honnef.co/go/js/dom/v2 (package name dom)?

Just to confirm, you're effectively referring to a vgo v2 of honnef.co/go/js/dom?

[dmitshur]

The import path was conceived with Go modules (vgo) and semantic import versioning in mind, but for now, I pictured it as just a normal directory named v2 in the same repository on master branch, containing a dom package.

If I understand correctly, it should be compatible with module-less Go 1.10 and intersect well with Go modules after that's released (1.11 and onwards).

Does that sound like a reasonable approach @myitcv, or is there something you'd suggest changing? I'm not yet very familiar with Go modules and how one is supposed to create packages for it.

[myitcv]

Given the /v2 API is only going to be relevant for 1.11 and onwards, is there a reason to go for a subdirectory at all? You could do all that work on a separate branch and simply tag releases for v2.x.x, which module-aware Go 1.11 will pick up if the /v2 import path is used.