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

kill multi_json Dependencies #1274

Closed
wants to merge 1 commit into from
Closed

kill multi_json Dependencies #1274

wants to merge 1 commit into from

Conversation

u2
Copy link
Contributor

@u2 u2 commented Feb 12, 2016

@@ -15,6 +15,7 @@

#### Fixes

* [#1274](https://github.com/ruby-grape/grape/pull/1274): Kill multi_json Dependencies - [@u2](https://github.com/u2).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should say "Removed multi_json dependencies", lets not kill anything ;)

@dblock
Copy link
Member

dblock commented Feb 12, 2016

If this is to be merged, we need an UPGRADING entry that says that we no longer use MultiJson and provides a Json formatter example of how to put it back.

I am torn though. I have a project that has heavily benchmarked itself using oj, so I will probably want to use MultiJson to get that performance. Or maybe I'll want to benchmark it again, I am not sure. Right now Grape basically says "you can use any JSON library without any work", so effectively we're removing a feature.

Thoughts?

@wshatch
Copy link
Contributor

wshatch commented Feb 12, 2016

What exactly is the advantage of this? It seems that it can kill OJ support which, from my understanding, performs better than the standard json module.

@dblock
Copy link
Member

dblock commented Feb 12, 2016

@u2
Copy link
Contributor Author

u2 commented Feb 13, 2016

Yes, it comes from mikeperham blog Kill Your Dependencies.

Every dependency in your application has the potential to bloat your app, to destabilize your app, to inject odd behavior via monkeypatching or buggy native code. When you are considering adding a dependency to your Rails app, it’s a good idea to do a quick sanity check, in order of preference:

Do I really need this at all? Kill it.
Can I implement the required minimal functionality myself? Own it.
If you need a gem:

Does the gem have a native extension? Look for pure ruby alternatives.
Does the gem transitively pull in a lot of other gems? Look for simpler alternatives.

Less gem means less memory, less dependencies, and less bugs.And the promoting of performance of oj or other json gems is very tiny for grape.

And

All modern Rubies ship JSON as part of stdlib. Using the gem actually hurts multi-platform support due to build difficulties on Windows.

@dblock
Copy link
Member

dblock commented Feb 14, 2016

I am on the side of removing the multi_json dependency because the number people using something like OJ is on the decline. I do want to make it very easy to bring back the old behavior.

First, this definitely needs UPGRADING describing how to re-introduce something like OJ (a custom formatter for example).

Maybe we can add configuration to the formatter where you can give it the engine to use for formatting and parsing? Or maybe it can detect the presence of multi_json and use that where available? Not sure what's best. Another option would be a grape-multi-json gem that brings the old behavior back. What do you think?

@u2
Copy link
Contributor Author

u2 commented Feb 15, 2016

Yes, what about adding a configuration option like multi_json? The default value is false,the JSON codes will be the native JSON, if it's true and existing multi_json gem, it will act like before.

@dblock
Copy link
Member

dblock commented Feb 15, 2016

Maybe format :json, processor: JSON or format :json, processor: MultiJson?

@u2
Copy link
Contributor Author

u2 commented Feb 16, 2016

👍 It looks workable.

@dblock
Copy link
Member

dblock commented Oct 21, 2016

I am still very interested in this @u2!

@namusyaka
Copy link
Contributor

Any progress on this?

@dblock
Copy link
Member

dblock commented May 2, 2017

Replacing in favor of #1623.

@dblock dblock closed this May 2, 2017
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

Successfully merging this pull request may close these issues.

4 participants