Remove dependency on honnef.co/go/js/{dom, util} #15

fabian-z · 2016-03-27T14:22:31Z

This was discussed but not applied in #4. I did similar changes to current master HEAD and wanted to share this for discussion and reference. Currently working great in staging for my application, reducing the generated JS size by avoiding the dom package.

Reduces minified, non-gzip file size about ~ 220kb for my project.
Also removing honnef.co/go/js/util only resulted in saving additional ~ 2kb.

dmitshur · 2016-03-27T18:21:28Z

Hmm.

I've briefly looked over the implementation, everything looked good (but I'll look again more thoroughly and test it locally before giving my LGTM if we're going to merge).

I think it's a little unfortunate we have to "optimize" some Go packages by effectively embedding other existing, well tested and used packages, simply because of inability to effectively DCE.

On the other hand, this package is still just a wrapper around one API that the browser offers (WebSockets), and the use of low-level *js.Object stuff doesn't escape its API, so I think this may very well be valid thing to do.

I'd like to see what other people think (/cc @nightexcessive @neelance @dominikh @slimsag), but in general, I don't have good arguments for why we shouldn't merge this, since it can improve generated code size for users.

dmitshur · 2016-03-27T18:22:15Z

wrapper.go

+	ws.Call("removeEventListener", typ, listener, useCapture)
+}
+
+


Not gofmted.

Thanks for catching! Corrected :)

mjohnson9 · 2016-03-27T18:56:41Z

I have to agree with @shurcooL - no good reason not to merge this.

LGTM on a cursory read through.

dmitshur · 2016-03-27T20:12:14Z

wrapper.go

@@ -76,10 +74,6 @@ func New(url string) (ws *WebSocket, err error) {
 type WebSocket struct {
 	*js.Object

-	// Available events:
-	// open, error, close, message


IMO this information is very valuable and helpful. You should preserve it.

Move it to AddEventListener documentation.

That way, it's a lot easier to get started with this library, since you can recognize the event types it supports and get going.

Agreed, in bc008c5 I tried to create sensible function documentation for AddEventListener and RemoveEventListener which preserves this information directly in source (although it is also available for users in doc.go).

Looks great, thanks!

fabian-z · 2016-03-27T22:36:56Z

Thanks for the feedback. With my last minor fix in test/test/index.go (squashed into commit d44071f), the test suite is now passing:

dmitshur · 2016-03-28T07:40:39Z

@fabian-z I haven't mentioned it earlier, but thanks for the PR.

For future changes, please push separate new commits so we can review them, and squash at the end once it has LGTM and is ready to be merged (or we can squash when merging, it's fine). Otherwise, it's really hard/impossible to review changes on GitHub if you just force push newly rebased commits.

Have you tested this change locally? Which parts?

I've tried it locally right now, and I immediately ran into an issue where it wouldn't work at all for the Dial, net.Conn path, with the following error:

It was happening inside getFrameData. I added the following to debug it:

if frame.Get("data") == js.Undefined {
    js.Global.Get("console").Call("log", frame)
}

And I saw the problem, the frame wasn't a MessageEvent as it should've been, but rather its constructor:

Then I found where the problem lies. It's here:

 func (c *Conn) onMessage(event *js.Object) {
    go func() {
-       c.ch <- dom.WrapEvent(event).(*dom.MessageEvent)
+       c.ch <- event.Get("constructor")
    }()
 }

That's not valid. I think you misread what dom.WrapEvent does.

It checks the o.Get("constructor") in order to figure out the type of event it is, and then returns a wrapper around o itself - the event - and not o's constructor.

Since the Conn.onMessage method is known to receive Message event type only, it used to do a type assertion .(*dom.MessageEvent) effectively negating/ignoring the work that WrapEvent did to figure out the event type (/cc @dominikh). This is actually additional motivation to merge this PR (after fixing this bug, of course).

So, the correct change would be to do this:

 func (c *Conn) onMessage(event *js.Object) {
    go func() {
-       c.ch <- dom.WrapEvent(event).(*dom.MessageEvent)
+       c.ch <- event
    }()
 }

After doing that, it got the websocket connection working again in my testing.

You'll want to do that in another place for closeError and its Close event too.

dmitshur · 2016-03-28T07:50:13Z

conn.go

 		cleanStmt = "clean"
 	} else {
 		cleanStmt = "unclean"
 	}
-	return fmt.Sprintf("CloseEvent: (%s) (%d) %s", cleanStmt, e.Code, e.Reason)
+	return fmt.Sprintf("CloseEvent: (%s) (%d) %s", cleanStmt, e.Get("code"), e.Get("reason"))


e.Get("code") should be e.Get("code").Int() and e.Get("reason") should be e.Get("reason").String().

That way, you get correct error code printed:

Before this change, it was not printing the right thing:

(The Get("reason").String() part is not strictly need .String() since fmt.Sprintf will use the fact that *js.Object implements fmt.Stringer, but better to do it for consistency.)

fabian-z · 2016-03-28T10:57:26Z

@shurcooL You're welcome! Glad about your review and that I can eventually contribute my changeset.

Of course you are right in regard to commit squashing, I originally only wanted to keep the history clean in order to easily allow you to merge this PR (I did not anticipate functional issues). I will take this into consideration for future collaboration.

It makes me wonder why this bug did not appear in my staging environment (and did not make anything fail in the test suite). However, I have read up on the API docs and did the same debugging as you, naturally coming to the same conclusion.

The issues you brought up should be now fixed in 6b1885b.
(Test suite is passing with my latest commit, too.)

Looking forward to your feedback!

dmitshur · 2016-03-29T05:21:02Z

conn.go

@@ -25,25 +24,24 @@ func beginHandlerOpen(ch chan error, removeHandlers func()) func(ev *js.Object)

 // closeError allows a CloseEvent to be used as an error.
 type closeError struct {
-	*dom.CloseEvent
+	*js.Object


This is purely an optional style suggestion - it doesn't change behavior - but I think it'd be nicer.

Since you're already embedding the *js.Object into this struct, you might as well add the JS fields here too, and then you can use them in Error() method more nicely.

What I mean is that closeError can look more like the *dom.CloseEvent which it used to embed before this PR:

// closeError allows a CloseEvent to be used as an error. type closeError struct { *js.Object Code int `js:"code"` Reason string `js:"reason"` WasClean bool `js:"wasClean"` }

Then you can just do if e.WasClean {, fmt.Sprintf("CloseEvent: (%s) (%d) %s", cleanStmt, e.Code, e.Reason), etc. You'll need to do &closeError{Object: ev} too, of course.

What do you think of that idea?

Now that I think about it, maybe it's worth doing the same for message events? I.e., create a:

type messageEvent struct { *js.Object Data *js.Object `js:"data"` }

Then do c.ch <- &messageEvent{Object: event} and use frame.Data. I'm not quite sure if it's worth doing or not... Any thoughts on this?

I think making closeError embed the correct JS fields makes the code much more readable and concise (since it removes the need to have xx.Get(yy).Type() calls everywhere).
However I am unsure about the messageEvent change. It actually makes it more clear what is passed around, but makes the code a little more verbose. But I think this verbosity is worth the clarity brings around. I separated both changes in two commits, so you can review them separately. 😄

dmitshur · 2016-03-29T05:25:11Z

@fabian-z, the change you've done in 6b1885b is identical to the one I've done locally when investigating the issue. So it looks good.

Of course you are right in regard to commit squashing, I originally only wanted to keep the history clean in order to easily allow you to merge this PR (I did not anticipate functional issues). I will take this into consideration for future collaboration.

No problem, I think it's great to start by squashing/force pushing at first before the change takes shape, but stop once it's in the final review stage where reviewers want to see any followup diffs clearly, and it can finally be squashed at the end once given final LGTM (or the maintainers can just do that when merging the PR). So you did everything right. :)

I've just made one optional style suggestion above, take a look.

Aside from that, and with 6b1885b done, this LGTM and I think I'll merge soon, since no one has objected to it.

I've also been thinking more and I think it's very reasonable for wrappers to use raw *js.Object calls. They provide the glue, the interconnect between the JS world and Go world. It's okay for the wrapper to be a little more painful to write by not reusing existing lower level wrappers. As long as the end result API is nice and idiomatic Go.

I'll probably want to apply a similar change to https://github.com/gopherjs/eventsource wrapper too.

fabian-z · 2016-03-30T08:55:46Z

@shurcooL

With my latest commits 75ac80c and 02c47aa I implemented your suggestions for the closeError and messageEvent structs.

In this case, it really is a bit nicer to have the fields implemented directly in the struct instead of using xx.Get(yy).Type() all the time.
As usual, test suite passes fine. However, I could not yet manually verify the functionality of these latest changes. Looking forward to your review.

I've also been thinking more and I think it's very reasonable for wrappers to use raw *js.Object calls. They provide the glue, the interconnect between the JS world and Go world. It's okay for the wrapper to be a little more painful to write by not reusing existing lower level wrappers. As long as the end result API is nice and idiomatic Go.

This was part of my reasoning for this PR (together with squashing the size of my projects generated JS). Importing a third-party package with a giant load of type definitions isn't very much efficient if you only need one type, anyway.

fabian-z · 2016-03-31T14:38:12Z

I now tested the suggested style changes implemented in my last two commits, they are working fine for usage in my application.
If there is anything left for me to implement or correct after your review, please let me know! 😄

dmitshur · 2016-03-31T17:30:18Z

LGTM. Thanks a lot for contributing!

I'll merge this evening. Feel free to squash into 1 commit (and nitpick, remove the space from {dom, util} in commit message). Otherwise I can do that when merging this. Thanks again, this was well done.

dmitshur · 2016-04-01T03:58:00Z

Merged as eed78ad. Thanks again @fabian-z!

dmitshur · 2016-04-01T03:59:38Z

I've looked into applying a similar change to github.com/gopherjs/eventsource package, but quickly realized it only imports util package, not dom. That package is tiny, so trying to avoid importing it has no sizable benefits.

We could've left util here as well, but it doesn't seem to matter either way (if anyone has reasons to go one way over the other, feel free to share).

dominikh · 2016-04-01T04:00:56Z

One could argue that util is so tiny that it's not worth having as a dependency and that one should rather copy its contents.

fabian-z · 2016-04-01T07:03:50Z

@shurcooL Thanks for merging and for your spot-on review before. Appreciated working together with you and being able to contribute this!

fabian-z changed the title ~~Remove dependency on honnef.co/go/js/dom~~ Remove dependency on honnef.co/go/js/{dom, js} Mar 27, 2016

fabian-z changed the title ~~Remove dependency on honnef.co/go/js/{dom, js}~~ Remove dependency on honnef.co/go/js/{dom, util} Mar 27, 2016

dmitshur reviewed Mar 27, 2016
View reviewed changes

dmitshur reviewed Mar 28, 2016
View reviewed changes

dmitshur reviewed Mar 29, 2016
View reviewed changes

Remove dependency on honnef.co/go/js/{dom,util}

b1e102f

dmitshur closed this in eed78ad Apr 1, 2016

Remove dependency on honnef.co/go/js/{dom, util} #15

Remove dependency on honnef.co/go/js/{dom, util} #15

Uh oh!

Conversation

fabian-z commented Mar 27, 2016

Uh oh!

dmitshur commented Mar 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjohnson9 commented Mar 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabian-z commented Mar 27, 2016

Uh oh!

dmitshur commented Mar 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabian-z commented Mar 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur commented Mar 29, 2016

Uh oh!

fabian-z commented Mar 30, 2016

Uh oh!

fabian-z commented Mar 31, 2016

Uh oh!

dmitshur commented Mar 31, 2016

Uh oh!

dmitshur commented Apr 1, 2016

Uh oh!

dmitshur commented Apr 1, 2016

Uh oh!

dominikh commented Apr 1, 2016

Uh oh!

fabian-z commented Apr 1, 2016

Uh oh!

Uh oh!