Idiomatic Go implementation #1

mjohnson9 · 2014-10-27T19:19:58Z

I've taken the liberty to create an idiomatic Go implementation of this WebSocket wrapper, primarily for my own uses. As such, I felt it was appropriate to share the implementation.

WebSocket now implements io.Writer and most calls are blocking. I began to implement the io.Reader interface, but decided that the refactor needed for buffering would be more appropriate for a separate implementation.

Better console messages Better domain detection A second string echo test A binary echo test

dmitshur · 2014-10-28T03:12:47Z

Hi @nightexcessive, thanks a lot for the PR!

It is quite large, so I'll take some time to review it, but at first glance it looks good.

/cc @ajhager since he was working on this too, if I remember correctly.

dmitshur · 2014-10-28T03:15:15Z

I began to implement the io.Reader interface

Is it possible to do that, despite gopherjs/gopherjs#89? My understanding is that it cannot be done at this time, see gopherjs/gopherjs#89 (comment) specifically. Can you share some thoughts?

dmitshur · 2014-10-28T03:16:21Z

example/run_fresh

@@ -0,0 +1,4 @@
+#!/bin/sh
+
+cd "$( dirname "${BASH_SOURCE[0]}" )/server"


cd "$( dirname "${BASH_SOURCE[0]}" )/server"

What does that do? Why not simply cd ./server?

This changes to /script/directory/server. It's for the case when the script is executed with a working directory outside of the example directory. Perhaps a comment should be added?

mjohnson9 · 2014-10-28T08:29:29Z

Is it possible to do that, despite gopherjs/gopherjs#89?

It seems not, unfortunately. After that comment, I ended up trying to implement it again. I attempted to fix io.Pipe with a channel-based implementation of sync.Cond, but I ran into that bug.

... as per code review in #1

dmitshur · 2014-11-01T22:26:11Z

Ok, some thoughts after looking over nightexcessive/websocket/main.go...

See comments below.

dmitshur · 2014-11-01T22:26:52Z

main.go

+	Protocol       string `js:"protocol"`
+	// Use the CONNECTING, OPEN, CLOSING, and CLOSED constants defined above for
+	// ReadyState.
+	ReadyState uint16 `js:"readyState"`


First off, the comments refer to older const names, before they were renamed. However, I think the entire thing can be simplified by using the named type instead of uint16:

ReadyState ReadyState `js:"readyState"`

That way it's self-documenting and autocompletion will assist with getting/comparing ReadyState values. Just need to ensure GopherJS supports js struct field tags with named types (the underlying type is still uint16), but I expect it'll work.

dmitshur · 2014-11-01T22:29:24Z

The New API is now higher level and it blocks until the websocket has either successfully connected or error.

That's something I want to confirm is a good direction to go, and what kind of implications it will have on user programs.

It seems like a good idea, but I'll want to play with it and confirm it works well in practice. Do you have thoughts/comments?

dmitshur · 2014-11-01T22:31:33Z

main.go

+	URL        string `js:"url"`
+
+	ch     chan *receiveItem
+	openCh chan *js.Error


Implementation details thoughts.

I think it might be simpler to use one channel for all websocket-related events, including open/close/error and receive. Or at least use openCh for everything other than receive events.

I think that would allow fixing the hack below with the potential panic when closing openCh since it can be done in two paths.

But this is an implementation detail and can be fixed later.

dmitshur · 2014-11-01T22:50:50Z

Overall, I think the changes are in a good direction and I want to use them. There are still a few minor points I'd want to address, and some higher level decisions I want to think about.

Ideally, I'd like to merge this into this repo but on non-master branch and continue to work.

On the other hand, this repo does explicitly say in the README that the API is not yet finalized so it might be okay to just merge as is and continue tweaking it.

dmitshur · 2014-11-01T22:55:46Z

Another point, I think it's better to keep the example in a separate repo, perhaps under gopherjs/examples in a websocket subfolder. That would mirror the approach of go-gl organization, see https://github.com/go-gl/examples.

That would keep this repo more lightweight, focused and easier to integrate into a playground-like environment.

mjohnson9 · 2014-11-02T01:21:03Z

Sorry, I've just had harddrive crash galore. It'll be a few days before I can setup my dev environment and get back to this.

mjohnson9 · 2014-11-07T20:59:44Z

I'm back up and working on this again.

mjohnson9 · 2014-11-07T20:59:48Z

Another point, I think it's better to keep the example in a separate repo

I agree. Though, I believe that may be outside of the scope of this PR.

mjohnson9 · 2014-11-07T22:26:46Z

I've thought of another possible way to implement net.Conn, inspired by code.google.com/p/go.net/websocket. I'm going to attempt it in a separate branch.

mjohnson9 · 2014-11-08T00:37:22Z

I've got an initial (and sloppy) implementation up in my repo. Let me know what you think, and whether we should go with a net.Conn-style implementation or the type proposed in this PR.

dmitshur · 2014-11-08T04:38:51Z

@nightexcessive, I have the following suggestion. I think this PR is a great move forward, but it needs more work and there are things I'd like to try on it before accepting it as the canonical "master" API.

How about I add you as a collaborator to this repo, we can merge this into another branch and keep working on it here, and merge into master when we both feel it's completed. Does that sound good to you?

mjohnson9 · 2014-11-08T16:01:37Z

That sounds great. I'll move my work over to a dev branch and push it and the try-NetConn branch here when you're ready.

mjohnson9 · 2014-11-08T17:34:41Z

In the mean time, I'll create a ticket for discussion about the type of interface that we'd like to implement.

dmitshur · 2014-11-08T18:48:32Z

That sounds great. I'll move my work over to a dev branch and push it and the try-NetConn branch here when you're ready.

Sounds perfect! I've invited you to the Web Gophers team, which should give you push access to this repo. Please feel free to work on branches but don't merge to master before at least one LGTM from someone else. Thanks a lot!

In the mean time, I'll create a ticket for discussion about the type of interface that we'd like to implement.

Great, let's discuss it there.

mjohnson9 · 2014-11-08T18:50:26Z

I've created a dev branch and pushed my try-NetConn branch. I'm closing this PR and deleting my own repo.

The discussion issue is #2.

Michael Johnson added 7 commits October 27, 2014 14:53

Initial implementation

02c88d2

Improved example

a2ab055

Add documentation to main.go

d24bcef

Ignore example's build files

ac8be77

Change example import to GopherJS version

247316c

Add instructions to example page

7a47f2d

Improve client example

70e40b3

Better console messages Better domain detection A second string echo test A binary echo test

dmitshur reviewed Oct 28, 2014
View reviewed changes

Michael Johnson added 2 commits October 28, 2014 06:33

Fix readyState constants

8238fa7

... as per code review in #1

Add comments to the run scripts

c455300

... as per code review in #1

dmitshur mentioned this pull request Oct 30, 2014

Spotlight: Bindings and Frameworks gopherjs/gopherjs-book#7

Open

dmitshur self-assigned this Nov 1, 2014

dmitshur reviewed Nov 1, 2014
View reviewed changes

dmitshur removed their assignment Nov 2, 2014

Make WebSocket.ReadyState use the ReadyState constants

2822e13

Documentation improvements

812c73c

Michael Johnson added 2 commits November 8, 2014 11:54

More documentation improvements

4ee910b

Add ReadyState.String()

2d6498d

mjohnson9 mentioned this pull request Nov 8, 2014

What type of interface do we want to implement? #2

Closed

mjohnson9 closed this Nov 8, 2014

		@@ -0,0 +1,4 @@
		#!/bin/sh

		cd "$( dirname "${BASH_SOURCE[0]}" )/server"

Idiomatic Go implementation #1

Idiomatic Go implementation #1

Uh oh!

Conversation

mjohnson9 commented Oct 27, 2014

Uh oh!

dmitshur commented Oct 28, 2014

Uh oh!

dmitshur commented Oct 28, 2014

Uh oh!

dmitshur Oct 28, 2014

Choose a reason for hiding this comment

Uh oh!

mjohnson9 Oct 28, 2014

Choose a reason for hiding this comment

Uh oh!

mjohnson9 commented Oct 28, 2014

Uh oh!

dmitshur commented Nov 1, 2014

Uh oh!

dmitshur Nov 1, 2014

Choose a reason for hiding this comment

Uh oh!

dmitshur commented Nov 1, 2014

Uh oh!

dmitshur Nov 1, 2014

Choose a reason for hiding this comment

Uh oh!

dmitshur commented Nov 1, 2014

Uh oh!

dmitshur commented Nov 1, 2014

Uh oh!

mjohnson9 commented Nov 2, 2014

Uh oh!

mjohnson9 commented Nov 7, 2014

Uh oh!

mjohnson9 commented Nov 7, 2014

Uh oh!

mjohnson9 commented Nov 7, 2014

Uh oh!

mjohnson9 commented Nov 8, 2014

Uh oh!

dmitshur commented Nov 8, 2014

Uh oh!

mjohnson9 commented Nov 8, 2014

Uh oh!

mjohnson9 commented Nov 8, 2014

Uh oh!

dmitshur commented Nov 8, 2014

Uh oh!

mjohnson9 commented Nov 8, 2014

Uh oh!

Uh oh!