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

Replace multi_json with json #10576

Merged
merged 1 commit into from May 12, 2013
Merged

Conversation

sferik
Copy link
Contributor

@sferik sferik commented May 12, 2013

Since Rails 4 will require Ruby 1.9 and since Ruby 1.9 includes json in the standard library, multi_json is no longer necessary. I am a longtime maintainer of multi_json but will stop supporting the library in June.

This patch replaces multi_json with stdlib json. It will use the json gem if a newer version is available.

I have already submitted similar pull requests to execjs, sprockets, and uglifier, so multi_json need not be a dependency of Rails 4 applications. This should have the effect of making JSON parsing and generation somewhat faster by removing an abstraction layer that is no longer necessary.

@haileys
Copy link
Contributor

haileys commented May 12, 2013

👍 for killing multi_json

@vipulnsward
Copy link
Member

👍

wycats added a commit that referenced this pull request May 12, 2013
@wycats wycats merged commit a395c22 into rails:master May 12, 2013
@steveklabnik
Copy link
Member

🤘

rafaelfranca added a commit that referenced this pull request May 12, 2013
tenderlove added a commit that referenced this pull request May 14, 2013
* master: (61 commits)
  add tests for reset_calbacks
  Fixing build broken by this change
  Extract variable out of loop
  Updated comment to Rails 4
  Fixes NoMethodError: `alias_method_chain` when requiring just active_support/core_ext
  better error message when app name is not passed in `rails new`
  Code cleanup for ActionDispatch::Flash#call
  Fix typo: require -> requires
  Add CHANGELOG entry for #10576
  Merge pull request #10556 from Empact/deprecate-schema-statements-distinct
  Some editorial changes on the documentation.
  respond_to -> respond to in a message from AM::Lint
  specify that dom_(id|class) are deprecated in controllers, views are fine
  copy edits [ci skip]
  Fix class and method name typos
  Replace multi_json with json
  ruby -> Ruby
  Adding documentation to the automatic inverse_of finder.
  Improve CHANGELOG entry [ci kip]
  Call assume_migrated_upto_version on connection
  ...

Conflicts:
	activesupport/lib/active_support/callbacks.rb
@chuyeow
Copy link
Contributor

chuyeow commented May 15, 2013

Is it too late to get this backported to 4-0-stable?

Btw, love your approach, @sferik

@canweriotnow
Copy link

:shipit:

@teamon
Copy link

teamon commented May 15, 2013

What about oj and yajl support?

@nirvdrum
Copy link
Contributor

Not to speak for @teamon, but I think the problem is there are JSON documents that the json library cannot or will not parse. In these cases, using an alternative JSON processor is necessary. While those libraries certainly can be used directly, this change will break existing uses of ActiveSupport's JSON processing that have worked fine since the 2.x days. If swapping out JSON processors isn't going to be possible, there should probably be a caveat in the upgrade guide that this could break existing apps.

@haileys
Copy link
Contributor

haileys commented May 15, 2013

@nirvdrum If there are JSON documents that are legal according to the JSON spec that json cannot parse, you should file a bug with json. If they are not legal according to the spec, then they are not JSON documents.

@nirvdrum
Copy link
Contributor

@charliesome Some it cannot parse because its architecture fundamentally doesn't support. E.g., docs of certain sizes will blow out memory in json but work with a very reasonable footprint in yajl. And json has deliberately hard-coded a depth limit in nested structures. I'd imagine that was done as a stop-gap to blowing out memory, but it's not an accidental bug. The error message very clearly tells you it will not parse that document (see: https://github.com/flori/json/blob/master/lib/json/common.rb#L119).

That aside, even if it could subsume all other JSON processors in some future release, then you'd have to require a version not bundled with stdlib anyway, kinda negating the purported benefit of this being bundled in stdlib.

@haileys
Copy link
Contributor

haileys commented May 15, 2013

@nirvdrum That's a fair point, I hadn't considered that. It seems to me that large documents requiring a 'SAJ' (is that a thing?) parser are probably out of scope of what Rails should handle for you, given that it's likely you'll need to work with them specially anyway.

@nirvdrum
Copy link
Contributor

I think if you know from the outset that's a limitation, it's a fair point. The way I've seen it happen is a bunch of code is already using ActiveSupport's JSON interface, a problematic doc comes along, and then the engine is swapped out. Regardless of the rationale, the point I was raising is that json is not a drop-in replacement for other providers but the ActiveSupport interface is going to look the same and as a result code that previously worked no longer will. My preference is to retain the ability to swap out the engine, whether that be with multi_json or by other means. But, barring that, I think it's misleading to indicate that a multi_json-like lib is no longer necessary and thus safe to remove. Something in the client app will have to be rewritten and mentioning that in either the CHANGELOG or upgrade guide probably makes sense (admittedly, I don't which one makes the most sense).

@mhenrixon
Copy link

But I want to use OJ for performance reasons how do I go about that now?

@jlecour
Copy link
Contributor

jlecour commented Oct 2, 2013

I'd also like to use a different JSON library than Ruby's default, within Rails.

For the moment, I'm working on an API, built with Rails 4 and ActiveModel::Serializers and I'd really want to use Oj or Yajl.

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.

None yet