taoensso / sente

Realtime web comms library for Clojure/Script
https://www.taoensso.com/sente
Eclipse Public License 1.0
1.73k stars 193 forks source link

Decomplect -- see jetty9-websockets-async #9

Closed ToBeReplaced closed 10 years ago

ToBeReplaced commented 10 years ago

At the risk of sounding rude, I want to point out to you the reasons why I will not be using this library. You're a really sharp guy, and rather than keep this feedback to myself, sharing it makes it more likely that we will agree on implementation in the future so we can both benefit. We want the same things, so why not talk about them?

First, you've mandated shipping edn. What if my frontend team writes JS and we want to ship JSON? What if we're high-throughput and are actually shipping BSON? Why can't I pick my serialization mechanism?

Where are my go-loops? You start go-loops on channels as in ch-pull-ajax-hx-chs, but you don't hand them back to me. What if I am trying to test my code and want to verify that the processing has stopped after I have closed my channel?

ajax-post-fn and ajax-get-or-ws-handshake-fn don't belong in the same place. These are different scenarios and should be treated as such.

Why can't I specify my own error handling? Why are you logging for me? I should be able to see successes/failures on channels and be able to make the decision myself.

Why is the cljs client related to the server at all? Reconnect logic in javascript is different than reconnect logic in java and is only relevant on the client side.

You are mandating authentication by using ring sessions. What if I want to generate random urls and use those? In fact, this is what the most recent application I'm working on needs to do.

You explicit recommend placing the resulting channels and functions as top level vars. This is terrible for testing -- where are your tests!

The overarching theme is that you are doing way too many things and making them non-configurable. Contrast this with my library: https://github.com/ToBeReplaced/jetty9-websockets-async

It's less than a quarter of the size of your code base, yet is entirely configurable in all of the above ways. Moreover, I simply return a javax.http.servlet.HttpServlet. This means that my libarary will work with any java server with servlets. Jetty/Tomcat/whatever. 100% test coverage. There's still a little bit of work to do to open up a few more places (ex. passing the client disconnect message), but the place to do that is clear and won't disrupt anyone by design.

As a final comment, you need to get your dependencies under control -- Why on earth is sente bringing in a redis client? I would not be able to bring this in to a production environment. For reference, lein deps :tree prints out:

WARNING!!! version ranges found for:
[com.keminglabs/cljx "0.3.2"] -> [org.clojars.trptcolin/sjacket "0.1.0.3"] -> [org.clojure/clojure "[1.3.0,)"]
Consider using [com.keminglabs/cljx "0.3.2" :exclusions [org.clojure/clojure]].
[com.keminglabs/cljx "0.3.2"] -> [org.clojars.trptcolin/sjacket "0.1.0.3"] -> [net.cgrand/regex "1.1.0"] -> [org.clojure/clojure "[1.2.0,)"]
Consider using [com.keminglabs/cljx "0.3.2" :exclusions [org.clojure/clojure]].
[com.keminglabs/cljx "0.3.2"] -> [org.clojars.trptcolin/sjacket "0.1.0.3"] -> [net.cgrand/parsley "0.9.1"] -> [org.clojure/clojure "[1.2.0,)"]
Consider using [com.keminglabs/cljx "0.3.2" :exclusions [org.clojure/clojure]].
[com.keminglabs/cljx "0.3.2"] -> [org.clojars.trptcolin/sjacket "0.1.0.3"] -> [net.cgrand/parsley "0.9.1"] -> [net.cgrand/regex "1.1.0"] -> [org.clojure/clojure "[1.2.0,)"]
Consider using [com.keminglabs/cljx "0.3.2" :exclusions [org.clojure/clojure]].
[com.taoensso/timbre "3.1.0"] -> [com.taoensso/encore "0.8.0"] -> [com.keminglabs/cljx "0.3.2"] -> [org.clojars.trptcolin/sjacket "0.1.0.3"] -> [org.clojure/clojure "[1.3.0,)"]
Consider using [com.taoensso/timbre "3.1.0" :exclusions [org.clojure/clojure]].
[com.taoensso/timbre "3.1.0"] -> [com.taoensso/encore "0.8.0"] -> [com.keminglabs/cljx "0.3.2"] -> [org.clojars.trptcolin/sjacket "0.1.0.3"] -> [net.cgrand/regex "1.1.0"] -> [org.clojure/clojure "[1.2.0,)"]
Consider using [com.taoensso/timbre "3.1.0" :exclusions [org.clojure/clojure]].
[com.taoensso/timbre "3.1.0"] -> [com.taoensso/encore "0.8.0"] -> [com.keminglabs/cljx "0.3.2"] -> [org.clojars.trptcolin/sjacket "0.1.0.3"] -> [net.cgrand/parsley "0.9.1"] -> [org.clojure/clojure "[1.2.0,)"]
Consider using [com.taoensso/timbre "3.1.0" :exclusions [org.clojure/clojure]].
[com.taoensso/timbre "3.1.0"] -> [com.taoensso/encore "0.8.0"] -> [com.keminglabs/cljx "0.3.2"] -> [org.clojars.trptcolin/sjacket "0.1.0.3"] -> [net.cgrand/parsley "0.9.1"] -> [net.cgrand/regex "1.1.0"] -> [org.clojure/clojure "[1.2.0,)"]
Consider using [com.taoensso/timbre "3.1.0" :exclusions [org.clojure/clojure]].

Possibly confusing dependencies found:
[com.taoensso/timbre "3.1.0"] -> [com.draines/postal "1.11.1"] -> [commons-codec "1.7"]
 overrides
[com.taoensso/timbre "3.1.0"] -> [com.taoensso/carmine "2.4.6"] -> [commons-codec "1.9"]

Consider using these exclusions:
[com.taoensso/timbre "3.1.0" :exclusions [commons-codec]]

 [clojure-complete "0.2.3" :exclusions [[org.clojure/clojure]]]
 [com.cemerick/austin "0.1.4"]
   [com.cemerick/piggieback "0.1.3"]
 [com.cemerick/clojurescript.test "0.2.2"]
 [com.keminglabs/cljx "0.3.2"]
   [org.clojars.trptcolin/sjacket "0.1.0.3"]
     [net.cgrand/parsley "0.9.1"]
     [net.cgrand/regex "1.1.0"]
   [org.clojure/core.match "0.2.0"]
   [watchtower "0.1.1"]
 [com.taoensso/encore "0.9.2"]
 [com.taoensso/timbre "3.1.0"]
   [com.draines/postal "1.11.1"]
     [commons-codec "1.7"]
     [javax.mail/mail "1.4.4" :exclusions [[javax.activation/activation]]]
   [com.taoensso/carmine "2.4.6"]
     [commons-pool "1.6"]
     [org.clojure/tools.macro "0.1.5"]
   [com.taoensso/nippy "2.5.2"]
     [org.iq80.snappy/snappy "0.3"]
     [org.tukaani/xz "1.4"]
   [io.aviso/pretty "0.1.8"]
   [org.clojure/data.fressian "0.2.0"]
     [org.fressian/fressian "0.6.3"]
   [org.clojure/tools.logging "0.2.6"]
   [org.xerial.snappy/snappy-java "1.1.1-M1"]
 [expectations "1.4.56"]
   [erajure "0.0.3"]
     [org.mockito/mockito-all "1.8.0"]
   [junit "4.8.1"]
 [http-kit "2.1.17"]
 [org.clojure/clojure "1.6.0-beta1"]
 [org.clojure/clojurescript "0.0-2173"]
   [com.google.javascript/closure-compiler "v20131014"]
     [args4j "2.0.16"]
     [com.google.code.findbugs/jsr305 "1.3.9"]
     [com.google.guava/guava "15.0"]
     [com.google.protobuf/protobuf-java "2.4.1"]
     [org.json/json "20090211"]
   [org.clojure/data.json "0.2.3"]
   [org.clojure/google-closure-library "0.0-20130212-95c19e7f0f5f"]
     [org.clojure/google-closure-library-third-party "0.0-20130212-95c19e7f0f5f"]
   [org.mozilla/rhino "1.7R4"]
 [org.clojure/core.async "0.1.278.0-76b25b-alpha"]
 [org.clojure/tools.nrepl "0.2.3" :exclusions [[org.clojure/clojure]]]
 [org.clojure/tools.reader "0.8.3"]
 [reiddraper/simple-check "0.5.6"]
ptaoussanis commented 10 years ago

As a final comment, you need to get your dependencies under control -- Why on earth is sente bringing in a redis client?

Ahh, thank you! That's a bug. Will try get an update out tonight.

Cheers!

ptaoussanis commented 10 years ago

Okay - quick follow up: seems it isn't a bug. You can check the deps over here: https://clojars.org/com.taoensso/sente

org.clojure/clojure 1.5.1
org.clojure/clojurescript 0.0-2173
org.clojure/core.async 0.1.278.0-76b25b-alpha
org.clojure/tools.reader 0.8.3
com.taoensso/encore 0.9.2
com.taoensso/timbre 3.1.1
http-kit 2.1.17

The issue is that lein deps runs under your :dev profile by default, which is pulling in the project's own :dev profile. This won't be a problem in production. From lein help profiles:

By default the :dev, :provided, :user, :system, and :base profiles are activated for each task, but their settings are not propagated downstream to projects that depend upon yours.

I prefer including the :dev stuff in this case since the project uses Cljx which some folks may not have had experience with. Did have an actual issue a few days ago though, so good you brought it to my attention to check.

Hope that helps!