Comments (14)
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.
from go-js-dom.
@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.
from go-js-dom.
@shurcooL thank you very much for your feedbacks. PR #59 implements the changes you have described above recommended.
from go-js-dom.
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:
- merged the GopherJS and WebAssembly .go files into one (i.e., kept the
syscall/js
implementation only) - moved the code into a
v2
directory so that its import path will behonnef.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
- converted all struct fields with
js:"foo"
tags to equivalent methods; a change that was necessary because thesyscall/js
API does not support thosejs:"foo"
tags - made
js.Value
embedded inBasicNode
(instead of being not embedded and calledObject
)
- I was able to work around the name collision by using the
Underlying()
method in a few places where the underlyingValue
was needed but was shadowed by aValue() string
method
- fixed the implementation of methods that work with callbacks to use the
js.Func
types rather thanfunc(*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).
from go-js-dom.
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.
from go-js-dom.
@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.
from go-js-dom.
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.:
Lines 1802 to 1811 in 6da835b
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.
from go-js-dom.
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.
from go-js-dom.
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.
from go-js-dom.
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.
from go-js-dom.
Ugh. Fine.
from go-js-dom.
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
?
from go-js-dom.
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.
from go-js-dom.
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.
from go-js-dom.
Related Issues (20)
- Using HTML5 history gives a nil reference error HOT 1
- "go get -u honnef.co/go/js/dom" fails HOT 2
- go get honnef.co/go/js/dom HOT 1
- DispatchEvent of "Click"/"Change" HOT 1
- HTMLTemplateElement is missing?
- Feature Request: Element.closest()
- Missing: HTMLDialogElement, HTMLSlotElement, HTMLPictureElement HOT 1
- WheelEvent inherits MouseEvent
- Cannot load honnef.co/go/js/dom/v2: no Go source files HOT 1
- feature request: indexeddb interface
- adoptNode is missing "deep" param
- v2 does not compile with go1.14 HOT 2
- v2: CreateTextNode() panics HOT 3
- v2: unable to DrawImage HOT 3
- v2: RequestAnimationFrame callback argument type misleading HOT 1
- Missing `Children` method on `dom.Element`
- Adding event type strings to use with `AddEventListener`
- go install honnef.co/go/js/dom/v2@latest yields an error message: HOT 1
- Add Element.ReplaceChildren HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from go-js-dom.