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

v2: Create with support for both GopherJS and GOOS=js GOARCH=wasm. #57

Open
dmitshur opened this Issue Jun 20, 2018 · 13 comments

Comments

Projects
None yet
5 participants
@dmitshur
Collaborator

dmitshur commented Jun 20, 2018

Go is getting Wasm support in 1.11: https://tip.golang.org/doc/go1.11#wasm.

A lot of frontend Go code uses this package, so supporting js/wasm would be very helpful.

@yml

This comment has been minimized.

yml commented Jul 4, 2018

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

This comment has been minimized.

Collaborator

dmitshur commented Jul 5, 2018

@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

This comment has been minimized.

yml commented Jul 5, 2018

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

@dmitshur

This comment has been minimized.

Collaborator

dmitshur commented Jul 6, 2018

@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

This comment has been minimized.

Collaborator

dmitshur commented Jul 8, 2018

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

This comment has been minimized.

yml commented Jul 9, 2018

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.

@cryptix

This comment has been minimized.

cryptix commented Jul 9, 2018

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.

@dominikh

This comment has been minimized.

Owner

dominikh commented Jul 9, 2018

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

This comment has been minimized.

Collaborator

dmitshur commented Jul 10, 2018

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

This comment has been minimized.

Owner

dominikh commented Jul 10, 2018

Ugh. Fine.

@myitcv

This comment has been minimized.

myitcv commented Jul 10, 2018

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

This comment has been minimized.

Collaborator

dmitshur commented Jul 10, 2018

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.

@dmitshur dmitshur changed the title from Support GOOS=js GOARCH=wasm. to v2: Create with support for both GopherJS and GOOS=js GOARCH=wasm. Jul 10, 2018

@myitcv

This comment has been minimized.

myitcv commented Aug 12, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment