-
Notifications
You must be signed in to change notification settings - Fork 25
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
Conversation
Also added a new function, websocket.NewWithGlobal, for custom implementations of the WebSocket interface (e.g. SockJS)
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 |
Can you give an example of a situation where this would be used? Edit: I see you provided SockJS as an example of that. |
#3 has been merged, so we can start thinking about applying positive changes from here to close this. |
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 Note that importing Anyway, I will look into it/think about it. |
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. I told Richard about this and we'll see if/how it can get optimized :-) |
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. |
|
I see. What if I don't use |
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. |
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.
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. |
This merge adds/changes a few things:
websocket.NewWithGlobal
function to allow specifying the variable on the window.websocket.WebSocket
from a struct to an interface.OnError
function.dom.MessageEvent
withjs.Object
to avoid huge file sizes (see this issue over at gopherjs/gopherjs)