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

wasm: Port of the library using `sycall/js` #59

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@yml

yml commented Jul 5, 2018

Status: This branch is able to compile and run simple program on GOOS=js GOARCH=wasm

The strategy for this branch follows Dimitry's recomendation.
The advantage is that their is no backward incompatible chnages for gopherjs. The downside is that their is a lot of duplicated code that will need to be kept in sync going forward.
From an API perspective the only incompatible changes is LastModified it now returns an error.

func (d *htmlDocument) LastModified() (time.Time, error) {
...
}

Another unfortunate source of awkwardness is the fact that I had to Named the embded struct to avoid the name conflict with the value in HTMLButtonElement.

I have no prior experience with using this library nor with the gopherjs compiler so everything need to be taken with a grain of salt..

Please let me know if there is any interest in merging this kind of changes in the main repo.

Resolves #57.

yml added some commits Jul 5, 2018

wasm: Add support for GOARCH=wasm
dom_wasm.go and event_wasm.go started has a copy of the gopherjs
version. I modified them to support `gopherwasm/js`.
wasm: Use `syscall/js` directly instead of `gopherwasm/js`
Since we have have dedicated files for wasm we can use `syscall/js`
directly.
@myitcv

This comment has been minimized.

myitcv commented Jul 5, 2018

The downside is that their is a lot of duplicated code that will need to be kept in sync going forward.

I think this package needs to shift towards being code generated from the HTML spec, which would mitigate this point, as well as allowing code generation of simple tests too. It would also allow us to remove the slightly awkward usage that comes from having embedded structs.

@dominikh

This comment has been minimized.

Owner

dominikh commented Jul 5, 2018

code generated from the HTML spec

🤣 good luck & have fun to whoever attempts that.

It would also allow us to remove the slightly awkward usage that comes from having embedded structs

How is it awkward? The DOM APIs can be modeled rather well with composition, actually, and the current Go API allows passing "base types" to functions, without having to rely on interfaces and a million getter methods.

@dominikh

This comment has been minimized.

Owner

dominikh commented Jul 5, 2018

Also, what happened to the idea of using github.com/hajimehoshi/gopherwasm for this, to avoid code duplication?

@myitcv

This comment has been minimized.

myitcv commented Jul 5, 2018

the current Go API allows passing "base types" to functions

My main gripe is actually more a function of the fact that in GopherJS, with *js.Object-special struct types you cannot use struct literals. Even without this restriction, you end up with deeply nested struct literals, when the "equivalent" HTML element is flat. This was my motivation for code generating "exploded" element property types for myitcv.io/react from a very reduced version of the HTML spec to produce things like AProps.

... without having to rely on interfaces and a million getter methods.

In the WASM world, don't we need to do everything with methods in any case? Because we don't have the *js.Object-special types of the GopherJS world which made it possible to use struct fields?

@dmitshur

This comment has been minimized.

Collaborator

dmitshur commented Jul 6, 2018

I think this package needs to shift towards being code generated from the HTML spec

Such a drastic change would need to be considered for v2 or another package. It’s not in scope of this PR/issue.

Also, what happened to the idea of using github.com/hajimehoshi/gopherwasm for this, to avoid code duplication?

The 2nd paragraph of the PR description and the linked comment in the issue describes the rationale.

@dominikh

This comment has been minimized.

Owner

dominikh commented Jul 6, 2018

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.

@yml

This comment has been minimized.

yml commented Jul 6, 2018

@dominikh Have you seen the initial implementation using gopherwasm/js ?

I see pros an cons to both approaches.

@dmitshur

dmitshur requested changes Jul 6, 2018 edited

Ok, I've tested the Wasm dom implementation in this PR with github.com/shurcooL/tictactoe, a small/medium-sized project that uses honnef.co/go/js/dom. After making some minor modifications, it was working, which is very promising:

image

I have 2 change requests for this PR so far, see inline comments.

dom_wasm.go Outdated
return els
}
// TODO (yml): Find out if we really want to modify this signature

This comment has been minimized.

@dmitshur

dmitshur Jul 6, 2018

Collaborator

We do not want to modify the signature. First, it causes the API to differ between GopherJS and Wasm. Second, it causes *htmlDocument to no longer satisfy the Document interface:

panic: interface conversion: *dom.htmlDocument is not dom.HTMLDocument: missing method LastModified

Third, converting thet lastModified Date to string and parsing it back to time.Time is not efficient.

Instead, let's use the same Date -> time.Time conversion process that GopherJS uses (it uses the Date.getTime method), which resolves all those issues:

func (d *htmlDocument) LastModified() time.Time {
	return time.Unix(0, int64(d.Object.Get("lastModified").Call("getTime").Int()) * 1000000)
}
dom_wasm.go Outdated
// new features are added to the DOM.
//
// If you depend on none of the APIs changing unexpectedly, you're
// advised to vendor this package.

This comment has been minimized.

@dmitshur

dmitshur Jul 6, 2018

Collaborator

For now, I'd prefer we avoid duplicating the package comment, since we can avoid it. Let's delete it here and in dom.go, and move it to a new file doc.go that just has the package comment and package clause.

Also, please move the import comment to just doc.go file (and remove it elsewhere).

wasm: Implemented the feedback from Dmitri Shuralyov
* Moved the comments to a doc.go
* Improved to the implementation of LastModified to make the signature
compatible
@yml

This comment has been minimized.

yml commented Jul 6, 2018

@shurcooL not sure I did the right thing to approve your changes. I pushed a commit in branch.
Thank you for the quick review and the guidance.

@dmitshur

not sure I did the right thing to approve your changes.

Your changes look good so far, thanks for making them!

Just one minor issue I spotted, see inline comment.

doc.go Outdated
// If you depend on none of the APIs changing unexpectedly, you're
// advised to vendor this package.
package dom // import "honnef.co/go/js/dom"

This comment has been minimized.

@dmitshur

dmitshur Jul 6, 2018

Collaborator

The package comment must be immediately preceding the package clause. There can't be a blank line separating them, otherwise it's not parsed as a package comment by godoc. So please remove the blank line on line 113.

Reference: https://golang.org/doc/effective_go.html#commentary.

dom_wasm.go Outdated
// new features are added to the DOM.
//
// If you depend on none of the APIs changing unexpectedly, you're
// advised to vendor this package.
package dom // import "honnef.co/go/js/dom"

This comment has been minimized.

@dmitshur

dmitshur Jul 6, 2018

Collaborator

Let's remove the import comment here as well, so it's only in doc.go file.

@yml

This comment has been minimized.

yml commented Jul 6, 2018

@shurcooL I did all the changes mentioned above.

This time I started to godoc -http=:6060 and I notice that by default if I do not pass GOOS=js GOARCH=wasm the index is empty. When I specify them I see the documentation as expected.

I am wondering if the very exact build target +build js,!wasm and +build js,wasm is not going to cause issue to code completion and the online godoc.org.

@dmitshur

This comment has been minimized.

Collaborator

dmitshur commented Jul 8, 2018

I did all the changes mentioned above.

Perfect, thanks!

I am wondering if the very exact build target +build js,!wasm and +build js,wasm is not going to cause issue to code completion and the online godoc.org.

That's a good catch. godoc.org has support for displaying docs for GopherJS-only packages (see golang/gddo#343), but I think it will treat the current code as an empty linux/amd64 package, displaying no docs other than the package comment.

To fix that, we can just add a // +build js constraint to doc.go to make it constrained to GopherJS and Wasm only. That way, godoc.org will use linux/js (GopherJS) mode for docs.

@dmitshur

This comment has been minimized.

Collaborator

dmitshur commented Jul 8, 2018

(Edit: Moved comment to issue #57.)

@myitcv

This comment has been minimized.

myitcv commented Jul 8, 2018

@shurcooL

However, I think I've found a potentially show-stopping problem.

This is what I was referring to in #59 (comment):

In the WASM world, don't we need to do everything with methods in any case? Because we don't have the *js.Object-special types of the GopherJS world which made it possible to use struct fields?

@dmitshur

This comment has been minimized.

Collaborator

dmitshur commented Jul 8, 2018

Sorry, I missed that part of your comment.

@dmitshur

This comment has been minimized.

Collaborator

dmitshur commented Jul 10, 2018

Thank you for working on this PR @yml. It has helped us better understand what it looks like to add Wasm support to this package and how viable it was to reuse the existing dom API. We found out that a (significant) breaking API change will be necessary after all, so we'll want to use a different import path.

I'll close this PR since we're not going to proceed with this exact approach, but much of the work done here can carry over into a future PR to implement the updated issue #57. Let's continue to do discussion/planning there.

@dmitshur dmitshur closed this Jul 10, 2018

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