Added sockjs support #97

Merged
merged 5 commits into from Aug 14, 2012

Projects

None yet

4 participants

@nelsonsilva
Contributor

I've added sockjs support. The server side sockjs is practically equal to browserchannel's so we should probably extract most of it to another class.
In order to use sockjs on the server just pass sockjs: {} in the options.
For the client I kept browserchannel as default (since it's the default on the server as well). Basically it only uses sockjs when browserchannel is not available. You can try it by removing bcsocket.js and adding http://cdn.sockjs.org/sockjs-0.2.1.js to one of the examples.

@josephg
Owner
josephg commented May 19, 2012

Nice work. I agree - we should pull all the server-side communication code out into its own file, and then we can plug sockjs / browserchannel / whatever in really easily. All those communication systems have almost the same API anyway.

Want to have a stab at that?

@josephg
Owner
josephg commented May 19, 2012

Also, this needs tests. It should be pretty easy to run the browserchannel tests (test/browserchannel.coffee) against the sockjs interface.

@nelsonsilva
Contributor

Thanks. I'll try to tackle it as soon as i have some free time. This will require looking at socketio to understand if there any major differences.
Regarding the tests i postponned them until the refactoring to reuse the code...

On a side note this is actually part of a bigger plan... get ShareJS working on vert.x... which i already managed.
The goal is to have a pristine ShareJS version working so i have custom versions of node, connect and sockjs-node all running in Rhino. The only changes i need to make to ShareJS are to not use coffeescript at runtime (point to ./lib instead of ./src and add prebuilt lib to cvs) and remove some requires (which should'nt even be there).
Regarding the ./lib vs ./src i was hopping you'd consider it along with a watch target for development.

@josephg
Owner
josephg commented May 21, 2012

Don't worry about socketio for now. Just refactoring it to make it work for browserchannel & sockjs would be a good start. Socket.io's sessions try to last forever (using cookies and stuff). When I tried to use it before there were bugs where a client would reconnect (using an old sessionid) without the server being notified. You'd just start getting more messages from a client which was supposed to have been closed.

I had a watch target for compiling to /lib for awhile. Its just coffee -bcwo lib src.

@nelsonsilva
Contributor

Joseph is this headed in the right direction ? Managed to extract practically all of it to a common session handling module.

@nelsonsilva
Contributor

I've added tests for sockjs. I'm using a websocket client and basically copied from browserchannel tests.

@camshaft

I've been using this PR for awhile now and it works really well. This would be awesome merged in.

@wmertens wmertens commented on the diff Aug 1, 2012
src/server/sockjs.coffee
+sessionHandler = require('./session').handler
+
+wrapSession = (conn) ->
+ conn.abort = -> @close()
+ conn.stop = -> @end()
+ conn.send = (response) -> @write JSON.stringify(response)
+ conn.ready = -> @readyState is 1
+ conn.on 'data', (data) -> @emit 'message', JSON.parse(data)
+ conn.address = conn.remoteAddress
+ conn
+
+
+exports.attach = (server, createAgent, options) ->
+ sjsServer = sockjs.createServer options
+ sjsServer.on 'connection', (conn) -> sessionHandler wrapSession(conn), createAgent
+ sjsServer.installHandlers server, {prefix: "/sockjs"}
@wmertens
wmertens Aug 1, 2012 Contributor

Shouldn't the prefix be configurable like for browserchannel? How about inserting options.prefix ?= "/sockjs" on line 17?

@wmertens
Contributor

Tested this and it works nicely. Pulling, we'll do that prefix thing later.

@wmertens wmertens merged commit 109b0b9 into josephg:master Aug 14, 2012
@wmertens
Contributor

Thanks BTW :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment