Separate internal and net.Conn implementations

[bign8]

Context: this is a follow up on the "no fmt" issues brought up recently. For those who aren't up to speed, feel free to check out https://github.com/gopherjs/websocket/issues/18 and https://github.com/gopherjs/websocket/pull/19.

Description: I dove into the import graphs, and as previously described, net and net/url also use the fmt package making it particularly difficult to remove the fmt dependency from the websocket package. On researching the code a little more, it appears the net packages are only used to fully implement the net.Conn interface. So this PR is separating the core JS wrapper logic from the net.Conn wrapper.

~The newly created websocket.go file exposes the internal js wrapper to the top level package. This should allow for existing library consumers to not break when updating to use this version. With my personal experience of upgrading versions of packages, it's usually good to just bite the bullet and do this abstraction (making this file un-necessary), but I put this file in place to mitigate the concerns of any sticklers for API compatibility issues.~

P.S: I am looking forward to collaborating with the gopherjs team!

Tasks

• [x] Propose a separation of internal and net.Conn code
• [x] Decide if this PR is a good idea
  • [x] Decide if websocket.go is necessary (it's not)
  • [x] ~Fix New to produce a WebSocket instead of a core.WebSocket~
  • [x] Add documentation so others can find gopherjs/websocket/core
• [x] Get Maintainers Approval
• [x] Profit!!!

Requested Reviewers

• @flimzy
• @shurcooL
• @nightexcessive

[flimzy]

If net is only included to reference its interfaces as return values, a simple solution is to duplicate those interfaces in the websocket package. See my simple diff.

I'm not suggesting this is necessarily a good idea, however. It means we have to stay on top of keeping those two interfaces (net.Conn and net.Addr) in sync with the standard library, any time there is a change. It also means it would become impossible (or at minimum, non-trivial) to support multiple versions of Go with the websocket package.

So, take this observation with a grain of salt. It's not a suggestion; it's only an observation.

I don't use this package, so I don't have any vested interest in any outcome here.

[bign8]

Good idea @flimzy! Only problem is that LocalAddr() and RemoteAddr() return a net.Addr interface, not a websocket.Addr. So I don't think the following assertion compiles correctly because those methods return different interfaces (even though they are the same). If I'm wrong, I think cloned interface method is a much better plan, it just requires home growing the url.Parse method. But I'm pretty sure this won't compile.

var _ net.Conn = (*conn)(nil)

Update: not a direct apply of your patch, but the basic idea applied

# github.com/gopherjs/websocket
./conn.go:19: cannot use (*conn)(nil) (type *conn) as type net.Conn in assignment:
*conn does not implement net.Conn (wrong type for LocalAddr method)
have LocalAddr() Addr
want LocalAddr() net.Addr
./conn.go:164: cannot use conn (type *conn) as type net.Conn in return argument:
*conn does not implement net.Conn (wrong type for LocalAddr method)
have LocalAddr() Addr
want LocalAddr() net.Addr

[dmitshur]

Thanks a lot for wanting to contribute, @bign8!

In general, I am very wary of these "no fmt" efforts. You can read https://github.com/golang/go/issues/15583 for more background information. But I understand, given how big net/http has become recently, the desire to try to keep it out if it's unused.

In order to be able to solve this task analytically:

• [ ] Decide if this PR is a good idea

Can you motivate it by posting the effects of doing this change. For example, post the generated file size of a program that imports websocket package before and after the change.

Once we have some concrete numbers of the savings, it's easier to think about how much effort we want to invest into re-arranging code.

[bign8]

Great idea @shurcooL; Let the experimentation begin!

Context

$ go version
go version go1.7.3 darwin/amd64
$ gopherjs version
GopherJS 1.7-1

Notes

• The following verbosity is to assert reproducibility.
• The // +build ignore prefix for the files is because main.go was in the root of this repository.

Tests

Master (control)

$ git rev-parse HEAD && gopherjs build main.go && ls -g main.js && gopherjs build -m main.go && ls -g main.js && cat main.go
edfe1438a4184bea0b3f9e35fd77969061676d9c
-rw-r--r--  1 staff  882744 Feb 24 23:23 main.js
-rw-r--r--  1 staff  608852 Feb 24 23:23 main.js
// +build ignore

package main

import "github.com/gopherjs/websocket"

func main() { websocket.New("ws://example.org/ws") }

No-FMT With Wrapper

$ git rev-parse HEAD && gopherjs build main.go && ls -g main.js && gopherjs build -m main.go && ls -g main.js && cat main.go
f2d9e22bd2e3459bce83243b82931cc05c9aa75f
-rw-r--r--  1 staff  884335 Feb 24 23:25 main.js
-rw-r--r--  1 staff  609973 Feb 24 23:25 main.js
// +build ignore

package main

import "github.com/gopherjs/websocket"

func main() { websocket.New("ws://example.org/ws") }

No-FMT With Core

$ git rev-parse HEAD && gopherjs build main.go && ls -g main.js && gopherjs build -m main.go && ls -g main.js && cat main.go
f2d9e22bd2e3459bce83243b82931cc05c9aa75f
-rw-r--r--  1 staff  75575 Feb 24 23:26 main.js
-rw-r--r--  1 staff  56145 Feb 24 23:26 main.js
// +build ignore

package main

import websocket "github.com/gopherjs/websocket/core"

func main() { websocket.New("ws://example.org/ws") }

Summary

	Master	Wrapper	Core
Normal Size	882,744	884,335	75,575
% Change*	882,744	0.18023345%	-91.43862773%
Minified Size	608,852	609,973	56,145
% Change*	608,852	0.18411699%	-90.77854716%

* Calculated with http://percent-change.com/

For those who are visually inclined (or because a real experiment requires plots 😉):

[mjohnson9]

Convincing results if you're only using websocket in your program, but I'm skeptical after reading the reasoning in golang/go#15583.

In an actual program, how often is fmt or net not imported? It seems that you'd have to carefully avoid most stdlib packages.

[bign8]

Excellent question @nightexcessive!

Of the 155 std packages, 83 of them uses fmt. source

Yes, larger programs have a higher likelihood of consuming the fmt package. But, for those of us not using the stdlib (or choosing to avoid it), the choice to implement the net.Conn interface does lead to the stdlib dependency chain.

One of my primary uses of gopherjs is to generate communication libraries between client and server modules. It does require some work and verification to assert the the compiled packages don't use the stdlib, but as seen above, the compiled size benefits are well worth a few minutes of my time.

The changes I'm currently proposing allow for those who don't care about the size of their generated source (or require the net.Conn implementation) to use the code as it stands; While those that choose to optimize can add /core to their github.com/gohperjs/websocket imports and have a significant size reduction (iff not otherwise entangling with stdlib packages).

I understand this topic easily degrades to a conversation between the utility and performance of code. Where, in this instance, utility is making websocket the Swiss army knife of code, supporting all possible uses and performance as the resulting compiled js size. Where https://github.com/gopherjs/websocket/pull/20 would potentially allow the consumer to choose.

[mjohnson9]

Your argument is convincing. I will review the code shortly.

[mjohnson9]

Reviewed the code; only very minor changes requested.

[mjohnson9]

LGTM.

It seems that this offers flexibility for an important demographic without making the code significantly more difficult to maintain.

@shurcooL has voiced his concerns and, as the other primary contributor on this project, I'd want his approval before this is merged.

[dmitshur]

Thank you for providing those numbers (and graphs!) @bign8, that's very helpful.

I think based on that, and your rationale in https://github.com/gopherjs/websocket/pull/20#issuecomment-282492857, I'm in agreement that we can support a pure-JS wrapper package that doesn't pull in all of net/http.

Doing so is quite easy and I can imagine one that cares strongly about generated sizes maintaining their own modified fork of this library. But it seems to be easy enough for us to provide that here, in the upstream repo, so I don't mind doing that.

Let's figure out the best way to do it.

From your PR so far, it really seems that it's very viable to have a package that wraps the websocket API that doesn't even need fmt package.

If that weren't the case, I think the current setup of having main websocket package unmodified, providing both high- and low-level bindings, in addition to a separate package that's low-level only, would be the best/only way to go.

However, it seems to me so far that we can do better. The problem with the above setup is that IMO it becomes unfriendly towards people seeing the package for the first time, because they would think "I just want to use the low-level API, but it's provided in both packages... which of the two packages should I use?"

An idea I'd like us to consider is the following. Let's consider splitting the high- and low-level bindings into two separate packages, without overlap.

The idea is that users who want low-level only can use that small package, and those who want a high-level implementation that implements net.Conn can use the other package. There's no overlap, and it's very clean.

What I like about doing that is that it would IMO improve the high-level package by making its API smaller, it would be just the Dial method and nothing more.

We can update the documentation to have the high-level package describe how to use it, and move the low-level API docs into its own package. We can add a sentence that points out the low-level bindings, something like:

// There are low-level bindings that use the typical JavaScript idioms
// available in package websocketjs.

This brings me to another point, I'd like to find a better name for the low-level bindings than core. Core is very generic and doesn't mean anything. core.New doesn't sound like it creates a websocket. core.WebSocket is also not ideal.

It's kinda hard to think of a better replacement, but right now my idea is websocketjs. So the two packages could be:

• github.com/gopherjs/websocket - Package websocket provides high-level bindings for the browser's WebSocket API.
• github.com/gopherjs/websocket/websocketjs - Package websocketjs provides low-level bindings for the browser's WebSocket API.

I'm still not completely sure doing the split is better, because perhaps some people would find it friendlier to see both APIs in one package. But, hopefully that's not the case. What do you people think?

[bign8]

@shurcooL Updated as suggested (sorry for the delay).

[bign8]

@shurcooL All good suggestions!

Here are what the godoc pages look like locally (godoc -http=:<your-favorite-port-here>)

• Package List
• Individual Packages Pages
  • websocket
  • websocketjs

[dmitshur]

This looks good.

@bign8, I have a few minor documentation tweaks in mind. To apply them to skip round-trip review comments latency, I'll push them to this branch if you don't mind. Please look it over after and feel free to adjust or let me know what you think.

[dmitshur]

@bign8 I've pushed e990143, can you look over it and let me know how it looks to you. Read its commit message for rationale for changes.

I think this looks good to merge from my side now. Thanks a lot for your work on this.

[bign8]

👍 on https://github.com/gopherjs/websocket/pull/20/commits/149a4c9539865d4c617652fd4c5685d8a32f9c10 and https://github.com/gopherjs/websocket/pull/20/commits/e990143363264fe2da18e1f21ba9bbc1c4d9cbf0. Documentation looks awesome! thanks @shurcooL!

[dmitshur]

@nightexcessive Do you want to take another look after all the changes?

If it looks good to you too, let's merge this.

[dmitshur]

Oh, I just noticed that test/test/index.go hasn't been updated to use the new API. @bign8, can you work on that?

[bign8]

Sure! Let me see if I can get that running

[bign8]

@shurcooL and @nightexcessive Tests run fine on my machine now.

cd test
go get ./...
go generate ./...
go run ./server.go
open localhost:3000

[mjohnson9]

Will review ASAP.

[dmitshur]

Thanks @bign8 for contributing!