Open rome-user opened 1 year ago
The PR uses multimethods and a dynamic var to select the desired JSON implementation.
I haven't looked into it too much but it feels that a better alternative would be to add :encode-json
and :decode-json
keys to the client configuration map passed to build-http-client
.
I've changed my mind on my idea but never noted it here. I think in general "choosing your own JSON library" is anti-modular. If there are two libraries that depend on hato
, they may not agree on the JSON library to choose. Your idea may get around this, but it still enables the exact opposite of what I intended: multiple JSON codecs in a Clojure project
I'd say libraries that use Hato should provide their users an ability to specify a custom Hato client. It makes sense regardless of JSON processing.
Multiple JSON codecs in a Clojure project can be easily achieved with my approach - you can simply construct multiple Hato clients. A Hato client is not inherently a singleton, you specify it per-request. In fact, by default Hato builds a new client for each request.
There is a rather neat little trick that hato
can use to essentially drop the cheshire dependency, and use whatever json-lib is available on the classpath (e.g. data.json
, jsonista
, or in fact cheshire
). Consider a json.clj
with the following public Vars:
(def from-string
(or
(find-impl "cheshire" :from/string) ;; cheshire.core/parse-string
(find-impl "data.json" :from/string) ;; clojure.data.json/read-str
(find-impl "jsonista" :from/string) ;; jsonista.core/read-value
(no-lib-found!)))
(def from-stream
(or
(find-impl "cheshire" :from/stream) ;; cheshire.core/parse-stream
(find-impl "data.json" :from/stream) ;; clojure.data.json/read
(find-impl "jsonista" :from/stream) ;; jsonista.core/read-value
(no-lib-found!)))
(def to-string
(or
(find-impl "cheshire" :to/string) ;; cheshire.core/generate-string
(find-impl "data.json" :to/string) ;; clojure.data.json/write-str
(find-impl "jsonista" :to/string) ;; jsonista.core/write-value-as-string
(no-lib-found!)))
(def to-stream
(or
(find-impl "cheshire" :to/stream) ;; cheshire.core/generate-stream
(find-impl "data.json" :to/stream) ;; clojure.data.json/write
(find-impl "jsonista" :to/stream) ;; jsonista.core/write-value
(no-lib-found!)))
With data.json
on the classpath, evaluating from-string
gives #function[clojure.data.json/read-str]
. The downside is that if more than one of these 3 libs are on the classpath (which can/does happen), the choice will be the first match (in the hard-coded order). This might not be a problem for hato
itself (as it will have no json dependencies anymore), but might cause confusion to consumers. Whether that is acceptable or not, is kind of debatable and ultimately up to the maintainer. For what it's worth completing the rest of the namespace shown above is fairly trivial:
(defn- try-resolve [sym]
(try @(requiring-resolve sym)
(catch Exception _ nil)))
(defn- find-impl
[lib direction]
(case lib
"cheshire"
(case direction
:from/string (try-resolve 'cheshire.core/parse-string)
:from/stream (try-resolve 'cheshire.core/parse-stream)
:to/string (try-resolve 'cheshire.core/generate-string)
:to/stream (try-resolve 'cheshire.core/generate-stream))
"data.json"
(case direction
:from/string (try-resolve 'clojure.data.json/read-str)
:from/stream (try-resolve 'clojure.data.json/read)
:to/string (try-resolve 'clojure.data.json/write-str)
:to/stream (try-resolve 'clojure.data.json/write))
"jsonista"
(case direction
:from/string (try-resolve 'jsonista.core/read-value)
:from/stream (try-resolve 'jsonista.core/read-value)
:to/string (try-resolve 'jsonista.core/write-value-as-string)
:to/stream (try-resolve 'jsonista.core/write-value))
))
(defn- no-lib-found! []
(throw
(IllegalStateException.
"None of`cheshire`, `data.json`, or `jsonista` could be found on the classpath!")))
Hope this helps...
@p-himik I agree that being able to provide your own encode/decode functions when building the client, is in many ways the cleanest & most flexible approach. However, it doesn't really help with dropping the cheshire
dependency, unless these two keys were made mandatory both when building a client object, but also when issuing a plain request (which as you said, builds a new client every time), and doing so would be a breaking change ...
[EDIT] - apologies, it seems that hato does NOT depend on cheshire
directly, but rather, it sort of assumes/checks if it's there (via json-enabled?
). Even though this approach is not too different from the code in my earlier message, I find it kind of odd, from an API design point-of-view. If you think about it, whether json/transit etc is enabled (or not) is irrelevant...The real question is is json/transit required?, and that can be inferred from the args provided when building a client (or a request). For example, if you saw :as :json
in the params, then you would have to provide a json encoder/decoder. From an API perspective this would have been so much cleaner/explicit, and this ticket/PR would not exist. However, since the decision to find/resolve cheshire
dynamically has already been made, it wouldn't be too hard to extend it to three choices, and in fact, the hard-coded order of (or chesire data.json jsonista)
kind of fits here, because cheshire has always been the 'default'. The important thing is that, providing your own encode/decode fns (e.g. at the client/request level), none of that indirection should matter! In other words, there are two dimensions to this:
hato
do in the absence of any custom encode/decode fns? Currently it tries to resolve cheshire
, and could fail. It could additionally try to resolve data.json
(which many people use), and perhaps jsonista
. Nothing will change for classpaths that include cheshire
(backwards compatible), and classpaths that don't include it, might work (instead of straight failing). The only confusion that might arise is when both data.json
& jsonista
are on the classpath, and user actually wants jsonista
(but will get data.json
). Not providing a jsonista
fallback sidesteps this issue altogether. hato
do in the presence of custom encode/decode fns? it should honor them!
Addresses #52.
This is not ready to be merged; unit test are not written yet. Here is an example of how to use:
(require 'hato.optional.data-json)
.The current design introduces a dynamic variable
hato.middleware/*json-lib*
, which gets set when you load the optional JSON adapter, or you can bind it yourself!