search

ring-clojure / ring-json

Ring middleware for handling JSON
313 stars 47 forks source link

Making Cheshire dependency optional 😼 #70

Open marksto opened 3 years ago

marksto commented 3 years ago

Hi @weavejester!

This Ring middleware lib has a good design — it's succinct (minimalistic codebase and API) and does one thing (read/write JSON from/to req/resp) and does it well (in terms of performance).

Still, it strongly depends on a particular JSON mapping implementation library, a Cheshire in this case. And while it is apparently the most popular library for this purpose, there are numerous projects out there that use its alternatives, be it another Jackson-based metosin/jsonista or org.clojure/data.json with zero external dependencies. And, I hope you agree, it makes sense to (re)use the same tool for the same tasks within the same project (possibly, with the same set of configs).

I understand what could have caused such a choice before (historical reasons), but wouldn't it be better to leave this choice to the user themselves nowadays? This can be achieved by abstracting from the specific implementation and dynamically linking any supported library from the current classpath (much alike the clj-http.client does it when looks for Cheshire). We've achieved a similar goal in the Telegram Bot API client, so there's already a reference code anyone could review.

But for the ring-json, the task is a bit trickier due to the use of streams API and passing the mapping parameters along the way. And I'd propose to: a) split different mapper-specific implementations into different namespaces/files, if this is at all possible, to keep the codebase clean, and b) test the performance degradation with JMH and, if it is substantial, leave this library as it is (backed by Cheshire), perhaps providing alternatives (backed by the other two mappers) as separate artifacts.

Like to hear your thoughts!

Cheers, Mark

weavejester commented 3 years ago

The simplest solution is just to create another library that uses a different JSON dependency. I don't think there's a compelling reason to complicate things by trying to provide different back ends from the same package.

marksto commented 3 years ago

Well, yeah, maybe in the case of this particular library you are right. It's quite small and it can be easily replaced by plugging in a different lib with the same API but backed by a different JSON mapper. Actually, that was my "plan B", as I mentioned above. But when things come, for instance, to smth. like the clj-http I think it should be more configurable in this place. But that's a completely different story... Thank you, James!