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

v2: add dom package #69

Merged
merged 3 commits into from
May 26, 2019
Merged

v2: add dom package #69

merged 3 commits into from
May 26, 2019

Conversation

dmitshur
Copy link
Collaborator

@dmitshur dmitshur commented May 18, 2019

This change implements the starting point of a new v2 major version
of the dom package.

It is now implemented on top of the syscall/js API, which is supported
by Go WebAssembly and GopherJS (as of version 1.12-2). The design goal
has been to stay as close as possible to v1 API to make the transition
easier to make. The only changes that were done were out of necessity:

  • All struct fields with js:"foo" tags have been replaced with
    equivalent methods
  • Underlying() returns js.Value instead of *js.Object
  • AddEventListener() returns js.Func instead of func(*js.Object)

There is a remaining TODO in window.RequestAnimationFrame method
to make it so that canceling the callback via CancelAnimationFrame
makes it so the js.Func wrapper's Release method is called. Arranging
this is difficult (it requires either having an internal map and sync
primitive for tracking js.Func by IDs, or changing the return value of
the RequestAnimationFrame method from int to something where the
js.Func and its Release method can be accessed easily.

The current implementation is functional but may end up using more
memory in some situations. It's not blocking for the initial alpha
release, so it remains a TODO for now.

This implementation was based on work done by @yml in
Pull Request #59.

Fixes #57.

/cc @neelance @hajimehoshi @myitcv

This change implements the starting point of a new v2 major version
of the dom package.

It is now implemented on top of the syscall/js API, which is supported
by Go WebAssembly and GopherJS (as of version 1.12-2). The design goal
has been to stay as close as possible to v1 API to make the transition
easier to make. The only changes that were done were out of neccessity:

- All struct fields with `js:"foo"` tags have been replaced with
  equivalent methods
- Underlying() returns js.Value instead of *js.Object
- AddEventListener() returns js.Func instead of func(*js.Object)

There is a remaining TODO in window.RequestAnimationFrame method
to make it so that cancelling the callback via CancelAnimationFrame
makes it so the js.Func wrapper's Release method is called. Arranging
this is difficult (it requires either having an internal map and sync
primitive for tracking js.Func by IDs, or changing the return value of
the RequestAnimationFrame method from int to something where the
js.Func and its Release method can be accessed easily.

The current implementation is functional but may end up using more
memory in some situations. It's not blocking for the initial alpha
release, so it remains a TODO for now.

This implementation was based on work done in by @yml in
Pull Request #59.

Fixes #57.

Co-authored-by: Yann Malet <yann.malet@gmail.com>
@dmitshur dmitshur requested a review from dominikh May 18, 2019 02:37
Copy link
Owner

@dominikh dominikh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • All of the attribute setters are missing.
  • Can we document that v2 is an alpha, and that there might still be breaking changes? We'll have breaking changes due to callbacks.
  • Now that v2 has a go.mod, does v1 need a go.mod, too?

As for the issue with releasing callbacks: Please don't introduce a map to map integers to handlers. Instead, we should return an opaque type that contains all the necessary information to remove a callback and release the function. Yes, that deviates more from the v1 API, but the benefits hugely outweigh that.

@dmitshur
Copy link
Collaborator Author

All of the attribute setters are missing.

Thanks for catching that huge and embarrassing blunder. :) We do need to add methods for setting attributes too.

Can we document that v2 is an alpha, and that there might still be breaking changes? We'll have breaking changes due to callbacks.

Yes, we should make that more clear and visible. We reserve the right to make changes to v2, and the likelihood of that happening is inversely proportional to its age.

I don't plan to make any release version tags for now, so the final v2.0.0 version is still far away. It will be pseudo-versions only for now.

Now that v2 has a go.mod, does v1 need a go.mod, too?

As far as I know, it does not need one. Since it's missing, the go command will continue to implicitly create one that contains only a module statement, as it currently does.

(However, I've never tried this, so there's a small chance I'm wrong about this. But it's easily fixable if it turns out to be a problem.)

That said, we can certainly add one. But doing so is an action that is orthogonal to adding v2.

@myitcv
Copy link

myitcv commented May 18, 2019

Would it not be preferable to code generate this API?

https://gowebapi.github.io/

This was mentioned in a discussion in https://groups.google.com/d/msg/golang-nuts/v7JoKNGTsR4/F8CLi4sNBgAJ

My preference would be towards code generating from the IDL spec.

And I'd almost be tempted to direct efforts towards supporting gowebapi rather than duplicating the work here (unless I'm missing something as to how the two are different)

cc @martin-juhlin

@dmitshur
Copy link
Collaborator Author

dmitshur commented May 18, 2019

@myitcv You made this suggestion in #59 (comment) and it was discussed briefly there. It's a very big change in direction and I think it should be a separate issue rather than a comment on a PR that implements a concrete plan.

This dom package was created in January of 2014, and it currently has 215 known importers on godoc.org (many of them are small test packages). The goal of this PR is to resolve issue #57, that is add support for Go WebAssembly in addition to GopherJS.

To implement that goal, a breaking API change must be made, which is why the import path is changing its major version to v2. But the goal is to make the API change as small as possible, so it's as easy to migrate existing Go code that imports honnef.co/go/js/dom to import honnef.co/go/js/dom/v2 instead.

My preference would be towards code generating from the IDL spec.

Using code generation and the IDL spec is a valid approach to create a Go binding for the DOM API, but that's not the approach used by this project in v1. It's not in scope for v2, which was proposed and accepted in #57 (comment). It would have to be either v3 API (that we should discuss in a separate issue), or it can happen in a different project (as is already the case, I think that's the right approach).

Users can choose which Go package they prefer to use for their needs. This PR is about adding WebAssembly support for people who want to continue to use this existing dom package and its existing API in their projects.

@myitcv
Copy link

myitcv commented May 18, 2019

I think it should be a separate issue rather than a comment on a PR that implements a concrete plan

I'm raising the point here in case it makes sense for us to reconsider prior to any further work, but it sounds like you have decided on the approach you'd like to follow.

Update GetBoundingClientRect to return DOMRect instead of the legacy
ClientRect. A breaking API change was needed to make it return a pointer
rather than a value, so it was a good opportunity to also update the
type at the same time.

Document API stability; v2 is currently in alpha and more API changes
may be done before it exits alpha state.
@dmitshur
Copy link
Collaborator Author

@dominikh The last two comments should address all your points, PTAL.

@dominikh dominikh self-requested a review May 20, 2019 04:35
@dmitshur dmitshur merged commit ebc4cf9 into master May 26, 2019
@dmitshur dmitshur deleted the add-v2 branch May 26, 2019 01:13
dmitshur added a commit that referenced this pull request May 4, 2020
The syscall/js API has changed in Go 1.14 as described
at https://golang.org/doc/go1.14#wasm.
Start using it, but keep compatibility for Go 1.13 and older
by making copies of .go files and using build constraints.

Fixes #74.

Co-authored-by: Dmitri Shuralyov <dmitri@shuralyov.com>
GitHub-Pull-Request: #69
dmitshur added a commit that referenced this pull request Aug 8, 2023
The dom/v2 API tried to stay as close as possible to its original API,
but reimplemented on top of syscall/js API for increased compatibility.
This happened 4 years ago (see PR #69).

The "Alpha, more API changes may be done soon" stage was meant give us
room to make significant changes in case we needed to. Given how much
time has passed and it's been working okay, we're way past that stage
and at a point where significant breaking API changes would not happen
in v2. Update the README to say that now, even if it's way overdue.

Also update documentation links while here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v2: Create with support for both GopherJS and GOOS=js GOARCH=wasm.
3 participants