mikemintz / rethinkdb-websocket-server

Node.js WebSocket server that proxies to RethinkDB. Supports query validation.
MIT License
156 stars 22 forks source link

Return wsServer to make use of ws events #17

Closed khoerling closed 8 years ago

khoerling commented 8 years ago

Return the underlying WebSocketServer object. This behavior is subject to change if the underlying transport library used in rethinkdb-websocket-server changes.

khoerling commented 8 years ago

Hey Mike! Really diggin' your software, have integrated the client/server into a larger project with advanced needs and this should help others doing similarly.

mikemintz commented 8 years ago

@khoerling thank you for the contribution! Can you explain the use case for this? I think it'd be great to provide more functionality, but I worry that this makes the API much more closely tied with the implementation, and I'm not sure the ws websocket server is a standardized API in the same way that the websocket client is standardized.

khoerling commented 8 years ago

A great example would be for persistence, knowing when users come online and offline at the server. Another approach would be to re-emit or otherwise expose ourselves, methods for connect/disconnect, etc... I understand the API stability issue, too-- consider Primus?

Related, is there a best-practice for re-establishing lost connections on the client?

mikemintz commented 8 years ago

I don't think you need to expose the wsServer to get access to online/offline events.

You get the online event with the RethinkdbWebsocketClient.connect promise resolves.

You can get the offline event by calling connection.on('close', handler) on the connection resolved from that promise.

See react-rethinkdb's Session.js for an example of that.

Are there other types of events that might not be possible to get this way?

khoerling commented 8 years ago

Is there a way to get events on the server?

mikemintz commented 8 years ago

Oh lol I see, sorry I wasn't thinking very clearly, I guess that was more about re-establishing lost connections on the client.

You do get the connect event on the server side with sessionCreator, but not anything else.

But I think I'm fine returning the wsServer object here like you suggest. If we change to something like primus, it might have to be backward incompatible in other ways, so as long as we document it and version it clearly it's not the end of the world.

Could you update the PR to update the comment above export function listen to also say something like "Return the underlying WebSocketServer object. This behavior is subject to change if the underlying transport library used in rethinkdb-websocket-server changes."

khoerling commented 8 years ago

You got it-- should be updated! I'm with you on the API and offering stable abstractions, for sure open to help make strides there with a cleaner idea.

Seen this yet? It's very much in infancy, especially considering the client tooling you built with react: https://github.com/tjmehta/rethinkdb-primus

mikemintz commented 8 years ago

I don't see the PR updated, are you sure you pushed?

khoerling commented 8 years ago

That commit should really be there now, pardon...

mikemintz commented 8 years ago

I see the comment in the commit message, but could you add it to the source code too above, at the end of the comment above export function listen?

khoerling commented 8 years ago

Ahh, no problem-- it's added now. After some thought, the best way to proceed is likely exporting our own portable methods for WebSocketServer open/close, etc... to guarantee a more stable API, like you mentioned.