search

jarohen / chord

A library designed to bridge the gap between the triad of CLJ/CLJS, web-sockets and core.async.
439 stars 40 forks source link

Added wrapper for nodejs websocket server. #55

Open tgetgood opened 7 years ago

tgetgood commented 7 years ago

I added a chord.node namespace with a single function ws-server which is a drop in replacement for node's ws package, but the handler gets a chord bidi-ch instead of a websocket object.

A downside to this approach is that the APIs for http-kit and node are different. I think that working smoothly with existing node libraries is more important, but you might disagree.

I've been using this for a toy project and it's worked so far. Not tested in anything production like.

This reuses the existing http-kit server code and just wraps it in an invocation of ws.Server. It's great that adding a new server was basically trivial.

I also put some example usage in comments in the chord.node namespace.

tgetgood commented 6 years ago

Thanks for the review @jarohen. All good points, I've made most of the changes you suggested. The one exception is the ws-server fn. I rearranged the arg order to be more consistent, but I think it makes more sense to leave 3 separate arguments, since the ws-opts is a js object that generally comes from a third party (like node's built in http server). The chord config and the underlying server config are different worlds, so I'd just as soon leave them separate.

I moved my examples into the README with some context for installing the npm dependencies. Let me know what you think.

jarohen commented 6 years ago

Thanks Tom - apologies for the delay in getting back to you.

I'd prefer to keep one options map if possible - both for consistency with existing functions, and so that consumers don't have to remember which way around the options maps are. I take the point that it might be confusing to have the two sets of options (chord + ws) in one map - maybe that means the ws-opts name could be more descriptive? I can't think of anything beyond ws-lib-opts for the time being, though. Means callers calling that function as follows:

       (def express (nodejs/require "express"))
       (def https (nodejs/require "https"))
       (def app (express))

       ;; ...
       ;; Set up your routes and server logic however you please

       (def server (.createServer https app))

       (def websocket-server
         (ws-server my-handler {:format :json,
                                :ws-lib-opts server}))

(pulled from updated README)

I've documented this function up in https://github.com/jarohen/chord/blob/node-server/src/chord/node.cljs - if you're ok with that, I'll go ahead and merge?

tgetgood commented 6 years ago

As I think about it, I have less of a strong opinion about keeping them together or separate. It's fairly arbitrary either way and a consistent API is important. I don't think it needs to be :ws-lib-opts, we could stick with :ws-opts for simplicity, or :ws-server-opts to be very explicit. Other than that I'm happy with this.

Oh, and don't forget to update the other example in the README.