Skip to content
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

Making Cheshire dependency optional 😼 #70

Open
marksto opened this issue Jul 16, 2021 · 2 comments
Open

Making Cheshire dependency optional 😼 #70

marksto opened this issue Jul 16, 2021 · 2 comments

Comments

@marksto
Copy link

marksto commented Jul 16, 2021

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
Copy link
Member

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
Copy link
Author

marksto commented Jul 16, 2021

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants