gopherjs / websocket

websocket provides high- and low-level bindings for the browser's WebSocket API.
BSD 3-Clause "New" or "Revised" License
111 stars 25 forks source link

Implement net.Conn high-level interface, improve low-level interface. #3

Closed mjohnson9 closed 9 years ago

mjohnson9 commented 9 years ago

This is for discussion about merging the new wrappers into master.

Previous discussion was in #2.

mjohnson9 commented 9 years ago

@shurcooL

To continue the discussion from #2, What are your thoughts on testing? I chose QUnit for simplicity, but the wrapper is inconvenient and not idiomatic.

dmitshur commented 9 years ago

I don't have thoughts on testing. I didn't realize it'd even be possible for GopherJS bindings. Do any other bindings have tests?

I'd be okay without tests, so any tests are an improvement.

mjohnson9 commented 9 years ago

Do any other bindings have tests?

gopherjs/jquery has tests. It uses QUnit and is where I copied the boilerplate for the QUnit tests from.

dmitshur commented 9 years ago

I see. https://github.com/dominikh/go-js-xhr does not seem to have any.

dominikh commented 9 years ago

My packages really aren't a good indicator for testability, I'm not big on tests in general (one of my weaknesses.)

dmitshur commented 9 years ago

What remains to be done before this can be merged?

I've tried out both new APIs over at https://github.com/shurcooL/play/commit/9b272571435d535ed82371cefc94368f72ad3719 and it seems good to me.

I think once this is ready, we should merge it as is, and then work on taking positive improvements in #4 (but not all changes) and merging them into master.

mjohnson9 commented 9 years ago

Aside from exploring some better unit tests, I don't believe there's anything left.

dmitshur commented 9 years ago

What about documenting what events can be AddEventListener'ed and providing a short typical usage example as part of the package docs (for the WebSocket API)? Is that already done?

mjohnson9 commented 9 years ago

Indeed. They're documented as part of the package docs.

dmitshur commented 9 years ago

Looks great. I've updated the README to match, does that look all right?

I think we can merge this soon, yeah?

Oh, we should probably mention the license (MIT, something permissive just in case someone needs to ask). And work on adding support for Travis so tests can be run there.

mjohnson9 commented 9 years ago

I believe this is ready to merge. Before we do so, we may want to commit to a versioning standard (such as semver) and use Git tags (perhaps in the format proposed by gopkg.in?) to mark versions.

dmitshur commented 9 years ago

Yeah, I'm okay to try using gopkg.in. This will be my first time trying it, but I think it's mostly additive and doesn't take anything away.

So, we can make the original (current master) version into branch v0, the new version (current dev) into branch v1. And separate point releases can be tags? Is that how it goes?

mjohnson9 commented 9 years ago

Yeah. We'd tag the latest commit on the master branch as v0. If we feel that it's going to be stable after this merge, we tag that commit as v1. Then, any changes that don't break compatibility will be v1.1 (or v1.0.1, as appropriate per semver).

dmitshur commented 9 years ago

When is a good time to wrap up this work?

I think we can merge this PR into master first, and treat it like v0. Then we incorporate improvements from #4. Then give it a week or two, and if there's nothing outstanding, we tag it as v1.

I'm still not quite sure on how to use branches/tags for this gopkg.in stuff, since I don't have experience, and the only way to have experience is to start trying it.

master branch (default branch?) - latest development version. v0 tag (or branch?) - pretty useless, but it can be a "release candidate" for v1. v1 tag (or branch?) - first official stable release, to be made when things are good on master? v1.0.1 tag - first patch, etc. v1.1 tag - first minor update, etc. v2 tag - first major breaking API change.

Just... thinking this through.

Edit: I've seen that v1 and v2 and master are all branches on some projects, rather than v1 and v2 being tags. How does that work? E.g., see https://github.com/go-pipe/pipe/branches. (/cc @niemeyer if you have some spare cycles to offer tips/hints here.)

Edit 2: After talking with @slimsag, I got a better understanding of how gopkg.in should work. I think the plan should be to keep master as the latest development version, and create v1, v1.0.1, v1.1, v2, etc. tags when we decide to explicitly release those versions.

dmitshur commented 9 years ago

@nightexcessive, are you okay with using MIT license for this?

mjohnson9 commented 9 years ago

I'd prefer the BSD 3-Clause, primarily for its protections of the contributors' names.

dmitshur commented 9 years ago

Let's go with your preference, you wrote most of the new implementation. I am indifferent (and uneducated about the differences), as long as the license is permissive.

mjohnson9 commented 9 years ago

Awesome.

I'm no legal expert, but I believe they're almost the same sans the contributor name protections. :+1:

dmitshur commented 9 years ago

I would ask what "contributor name protection" means, but that would imply I care, which I don't think I do, so I will not ask.

Yes, this is me not asking what it means. :P

dmitshur commented 9 years ago

I think we should add that license, and merge this PR into master. We can then create a new issue about considering switching to gopkg.in as the primary import path and discuss that there. I think the issue is still somewhat complicated so I'd prefer to separate it from this PR and the other open PR we have to close first.

mjohnson9 commented 9 years ago

Sounds good!

dmitshur commented 9 years ago

@nightexcessive, anything you're waiting on? Can I help? I'd like to move forward on this whenever you are ready. :)

mjohnson9 commented 9 years ago

I'm not waiting on anything. I'll go ahead and merge this now!

mjohnson9 commented 9 years ago

All merged.

dmitshur commented 9 years ago

Excellent! Thank you! And congratulations! :)