-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added wrapper for nodejs websocket server. #55
base: master
Are you sure you want to change the base?
Conversation
Should be a drop in replacement for node's ws package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, @tgetgood :)
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.
Nope, I agree - although I've got a couple of suggestions below that hopefully maintain consistency with the rest of the API, and allow the flexibility of the underlying Node libs. Let me know what you think?
Thanks for the examples too - could you add a Node section in the README to help users discover this functionality?
Cheers!
James
src/chord/channels.cljc
Outdated
#?(:clj (http/on-close ws (fn [_] (close! ws-ch))) | ||
:cljs (.on ws "close" #(close! ws-ch)))) | ||
|
||
(defn wrap-websocket [socket {:keys [read-ch write-ch] :as opts} & [cleanup]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you coalesce the opts map and the cleanup function, so that we don't have multiple options parameters? e.g. (defn wrap-websocket [socket {:keys [read-ch write-ch on-close] :as opts}] ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a bit more thought, I wonder why we leave this up to the implementer at all? We only have two types of connections, so I can clean up automatically.
src/chord/node.cljs
Outdated
[chord.channels :as channels] | ||
[chord.format :as format])) | ||
|
||
(defn ws-server [ws-opts {:keys [read-ch write-ch :as chord-opts]} handler] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a more consistent signature here is (defn ws-server [handler {:keys [read-ch write-ch ws-opts]}]...)
? (looking at wrap-websocket-handler
, above)
src/chord/node.cljs
Outdated
(comment | ||
"Plain example" | ||
|
||
(defn handler [ws-ch req] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are great examples - could you also put them in the README to make them more discoverable?
Also rearranged args order to be more idiomatic.
They're in the README now.
Thanks for the review @jarohen. All good points, I've made most of the changes you suggested. The one exception is the I moved my examples into the README with some context for installing the npm dependencies. Let me know what you think. |
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 (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? |
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 Oh, and don't forget to update the other example in the README. |
I added a
chord.node
namespace with a single functionws-server
which is a drop in replacement for node's ws package, but the handler gets a chordbidi-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.