Separate internal and net.Conn implementations #20

bign8 · 2017-02-16T06:24:39Z

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 #18 and #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

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

Requested Reviewers

@flimzy
@shurcooL
@nightexcessive

flimzy · 2017-02-16T10:59:39Z

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 · 2017-02-16T14:56:03Z

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 · 2017-02-23T03:26:41Z

Thanks a lot for wanting to contribute, @bign8!

In general, I am very wary of these "no fmt" efforts. You can read golang/go#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 · 2017-02-25T06:19:18Z

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 · 2017-02-25T15:06:09Z

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 · 2017-02-25T15:59:12Z

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 #20 would potentially allow the consumer to choose.

mjohnson9 · 2017-02-25T16:34:15Z

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

mjohnson9

See code comments for requested changes

mjohnson9 · 2017-02-25T16:40:48Z

conn.go

@@ -13,6 +13,7 @@ import (
 	"time"

 	"github.com/gopherjs/gopherjs/js"
+	core "github.com/gopherjs/websocket/core"


Core prefix is not necessary

mjohnson9 · 2017-02-25T16:41:46Z

websocket.go

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


Core prefix is not necessary

mjohnson9 · 2017-02-25T16:42:26Z

Reviewed the code; only very minor changes requested.

mjohnson9 · 2017-02-25T17:04:10Z

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 · 2017-02-28T00:10:18Z

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

I think based on that, and your rationale in #20 (comment), 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 · 2017-03-02T04:00:12Z

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

dmitshur

Sorry for the delay in reviewing this.

This mostly looks good, but there are a few inconsistencies in the documentation that need to be ironed out. See inline comments.

Additionally, we should update the README to be up to date. It's fine to keep both examples there, just update the text to refer to each of the two packages. The current README assumes both bindings live in the same top-level package.

dmitshur · 2017-03-09T00:51:07Z

doc.go


-	ws, err := websocket.New("ws://localhost/socket") // Does not block.
+	ws, err := websocketjs.New("ws://localhost/socket") // Does not block.
 	if err != nil { panic(err) }

 	onOpen := func(ev *js.Object) {


I think we should move this entire example for websocketjs into that package, rather than keeping it here.

Addressed in 4293b3c

dmitshur · 2017-03-09T00:59:49Z

websocketjs/websocketjs.go

@@ -2,11 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be found
 // in the LICENSE file.

-package websocket


It can be something like this:

/* Package websocketjs provides low-level bindings for the browser's WebSocket API. These bindings allow for using typical JavaScript idioms, such as adding event listeners with callbacks. For example: ws, err := websocketjs.New("ws://localhost/socket") // Does not block. if err != nil { ... } onOpen := func(ev *js.Object) { err := ws.Send([]byte("Hello!")) // Send a binary frame. err := ws.Send("Hello!") // Send a text frame. } ws.AddEventListener("open", false, onOpen) ws.AddEventListener("message", false, onMessage) ws.AddEventListener("close", false, onClose) ws.AddEventListener("error", false, onError) err = ws.Close() if err != nil { ... } */ package websocketjs

I've also replaced if err != nil { panic(err) } with if err != nil { ... }. I think it's clear enough that it means you should handle errors, and it's better not to show panic(err) as a way of doing that.

Addressed in 4293b3c

dmitshur · 2017-03-09T01:09:51Z

doc.go

@@ -21,9 +21,9 @@ The high-level bindings act like a regular net.Conn. They can be used as such. F
 	err = c.Close()


At the very top, this package comment still says:

Package websocket provides high- and low-level bindings for the browser's WebSocket API.

It should be updated. How about:

Package websocket provides high-level bindings for the browser's WebSocket API. These bindings offer a Dial function that returns a regular net.Conn. They can be used similar to net package. For example: ...

You can replace if err != nil { panic(err) } with if err != nil { ... } here as well, if you're okay with that.

Addressed in 4293b3c

- Updating README.md to reflect websocket and websocketjs package distinctions - Moving low-level example to websocketjs - Replace documentation panic error handling with "..."

bign8 · 2017-03-12T15:56:34Z

@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 · 2017-03-12T17:58:46Z

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.

Use patterns similar to net/http package documentation for displaying error handling example. See https://godoc.org/net/http examples on top. Rename c to conn to make the high level example more readable. Try to adjust language to be more readable and consistent. Process buf[:n] data first, handle error second. This is correct according to Read method documentation, see https://godoc.org/io#Reader.Read: Callers should always process the n > 0 bytes returned before considering the error err. Doing so correctly handles I/O errors that happen after reading some bytes and also both of the allowed EOF behaviors. Remove outdated reference to Conn from websocketjs package.

dmitshur · 2017-03-12T18:16:49Z

@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.

flimzy · 2017-03-12T18:18:46Z

README.md

@@ -3,33 +3,42 @@ websocket

 Packages websocket and websocketjs provide high- and low-level bindings for the browser's WebSocket API (respectively).

-The high-level bindings act like a regular `net.Conn`. They can be used as such. For example:
+The high-level bindings offer a Dial function that returns a regular net.Conn.
+It can be used similar to net package.


I know everybody hates a grammar nazi, but this should read "It can be used similarly to net package", as it describes an action, so an adverb is called for.

I know everybody hates a grammar nazi

I do not, thank you for the suggestion for improvement. I am happy to apply it.

flimzy · 2017-03-12T18:19:11Z

doc.go

@@ -6,20 +6,24 @@
 Package websocket provides high-level bindings for the browser's WebSocket API.

 These bindings offer a Dial function that returns a regular net.Conn.
-They can be used similar to net package. For example:
+It can be used similar to net package.


Same comment here.

It describes an action, so an adverb is called for.

bign8 · 2017-03-12T19:28:14Z

👍 on 149a4c9 and e990143. Documentation looks awesome! thanks @shurcooL!

dmitshur

LGTM.

dmitshur · 2017-03-12T19:48:20Z

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

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

dmitshur · 2017-03-12T19:50:11Z

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 · 2017-03-12T19:52:29Z

Sure! Let me see if I can get that running

bign8 · 2017-03-12T20:02:46Z

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

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

mjohnson9 · 2017-03-12T20:15:26Z

Will review ASAP.

Re-reviewing

mjohnson9

LGTM. You're very thorough, @shurcooL, far moreso than myself. 👍

dmitshur · 2017-03-13T01:19:07Z

Thanks @bign8 for contributing!

bign8 added 3 commits February 15, 2017 22:42

Attempting to remove fmt via sub-packaging

35f6383

Moving conn.go back to parent level

1d19ed2

Adding export of methods of low level methods

f2d9e22

mjohnson9 requested changes Feb 25, 2017

View reviewed changes

Renaming core/wrapper.go package to core instead of websocket

9bffe92

mjohnson9 previously approved these changes Feb 25, 2017

View reviewed changes

bign8 added 2 commits March 1, 2017 19:41

Renaming core to websocketjs

f4d9d91

Updating documentation with high/low-level APIs

7ea9a8d

dmitshur requested changes Mar 9, 2017

View reviewed changes

Updating documentation for websocketjs

4293b3c

- Updating README.md to reflect websocket and websocketjs package distinctions - Moving low-level example to websocketjs - Replace documentation panic error handling with "..."

flimzy reviewed Mar 12, 2017

View reviewed changes

Use "similarly", an adverb.

149a4c9

It describes an action, so an adverb is called for.

dmitshur approved these changes Mar 12, 2017

View reviewed changes

Getting test runner synced with websocketjs changes

23ac36d

mjohnson9 self-requested a review March 12, 2017 20:15

mjohnson9 approved these changes Mar 12, 2017

View reviewed changes

dmitshur merged commit f7ee9fa into gopherjs:master Mar 13, 2017

dmitshur mentioned this pull request Mar 13, 2017

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

Closed


		package websocket

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

		@@ -21,9 +21,9 @@ The high-level bindings act like a regular net.Conn. They can be used as such. F
		err = c.Close()

Separate internal and net.Conn implementations #20

Separate internal and net.Conn implementations #20

Uh oh!

Conversation

bign8 commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flimzy commented Feb 16, 2017

Uh oh!

bign8 commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitshur commented Feb 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bign8 commented Feb 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Notes

Tests

Master (control)

No-FMT With Wrapper

No-FMT With Core

Summary

Uh oh!

mjohnson9 commented Feb 25, 2017

Uh oh!

bign8 commented Feb 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjohnson9 commented Feb 25, 2017

Uh oh!

mjohnson9 left a comment

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!

mjohnson9 commented Feb 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjohnson9 commented Feb 25, 2017

Uh oh!

dmitshur commented Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bign8 commented Mar 2, 2017

Uh oh!

dmitshur left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

bign8 commented Mar 12, 2017

Uh oh!

dmitshur commented Mar 12, 2017

Uh oh!

dmitshur commented Mar 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

bign8 commented Mar 12, 2017

Uh oh!

dmitshur left a comment

bign8 commented Feb 16, 2017 •

edited

Loading

bign8 commented Feb 16, 2017 •

edited

Loading

dmitshur commented Feb 23, 2017 •

edited

Loading

bign8 commented Feb 25, 2017 •

edited

Loading

bign8 commented Feb 25, 2017 •

edited

Loading

mjohnson9 commented Feb 25, 2017 •

edited

Loading

dmitshur commented Feb 28, 2017 •

edited

Loading

dmitshur commented Mar 12, 2017 •

edited

Loading