Skip to content

Allow for SockJS; removed dom import to reduce file sizes; made WebSocket an interface #4

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

Closed
wants to merge 4 commits into from

Conversation

liamcurry
Copy link

This merge adds/changes a few things:

  • Added websocket.NewWithGlobal function to allow specifying the variable on the window.
  • Added example usage to the readme.
  • Changed websocket.WebSocket from a struct to an interface.
  • Added OnError function.
  • Replaced dom.MessageEvent with js.Object to avoid huge file sizes (see this issue over at gopherjs/gopherjs)

Liam Curry added 3 commits December 14, 2014 00:02
Also added a new function, websocket.NewWithGlobal, for custom implementations
of the WebSocket interface (e.g. SockJS)
@liamcurry liamcurry changed the title Allow for other implementations such as SockJS Allow for SockJS; removed dom import to reduce file sizes; made WebSocket an interface Dec 14, 2014
@dmitshur
Copy link
Member

Hi, thanks for the PR, but you should be aware of #3 where we're working on a more complete version of this package. There are some good ideas in this PR we can adapt into there. /cc @nightexcessive

@dmitshur
Copy link
Member

Added websocket.NewWithGlobal function to allow specifying the variable on the window.

Can you give an example of a situation where this would be used? Edit: I see you provided SockJS as an example of that.

@dmitshur
Copy link
Member

#3 has been merged, so we can start thinking about applying positive changes from here to close this.

@dmitshur
Copy link
Member

I'd like to look into this. I am wondering if the dead code elimination is working as much as I expect it to. I've noticed (like you) that importing the dom package and using only a very small subset of its types/methods results in a huge increase in generated file size. I am wondering DCE can be more aggressive at eliminating unused things.

Note that importing dom only increases file size if it's not already imported in the user code. So while avoiding it in websocket bindings may be feasible, that may not work well in more complex user programs.

Anyway, I will look into it/think about it.

@dominikh
Copy link
Member

DCE isn't really to blame here. Almost all types in the dom package are alive, because they can be created dynamically at runtime (there is one type per HTML element.)

The majority of the file size right now is caused by GopherJS encoding method sets directly in the file, without any deduplication. dom makes heavy use of embedding, of rather large interfaces, which means that we have a ton of types with a ton of methods. In one of my projects, I have a 2.3 MB (not minified) JS file. Of that, 1.3 MB are just the method sets. Deduplicating these naively, (i.e. sort -u) brings it down to 284 KB, and we aren't even considering partial duplication yet (e.g. [foo, bar, baz] and [foo, bar, qux])

I told Richard about this and we'll see if/how it can get optimized :-)

@dmitshur
Copy link
Member

DCE isn't really to blame here. Almost all types in the dom package are alive, because they can be created dynamically at runtime (there is one type per HTML element.)

Thank you for that insight. I suspected there was a reason why those unused types could not be eliminated, but wasn't sure why.

By "created dynamically", do you mean using strings and reflect package? Otherwise if my code never mentions the types, it can't be created, right?

It's good to hear there is still opportunity to reduce the size of generated code through work on methods sets.

@dominikh
Copy link
Member

document.GetElementByID("foo") -- that might be an *HTMLDivElement or an *HTMLInputElement or any other *Element type.

@dmitshur
Copy link
Member

I see. What if I don't use GetElementByID?

@dominikh
Copy link
Member

Any function or method that returns a value of type Node, Element or HTMLElement or a struct type that includes one of these will go through the wrappers. That includes any event-related code, because the event target is part of the event.

dmitshur pushed a commit that referenced this pull request Apr 1, 2016
The dom package is large, so for users that don't already import it,
it will make this package much more lightweight. The approximate
amount of non-gziped, minified savings is on the order of 220 KB.

Not relying on dom package means having to repeat some of the work
it does (and increase the generated size for packages that _do_
import dom package, but by very little since there's not much overlap).
It's worth it since there's not much, and this library continues to
provide the same Go API as before.

Closes #15.
Updates #4.
@dmitshur
Copy link
Member

I'm going to close this PR because it's inactive and too out of date to be actionable.

If there's anything else that needs to be done, let's discuss that in a new issue and PR, we can reference this one as needed. Thanks for contributing.

@dmitshur dmitshur closed this Mar 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants