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

Avoid playing with Oj shared default_options #744

Open
BuonOmo opened this issue Sep 26, 2021 · 2 comments
Open

Avoid playing with Oj shared default_options #744

BuonOmo opened this issue Sep 26, 2021 · 2 comments

Comments

@BuonOmo
Copy link

BuonOmo commented Sep 26, 2021

Oj.default_options = { :mode => :compat, :time_format => :ruby, :use_to_json => true }

This line causes a lot of trouble for anyone using Oj and requiring rabl after having set Oj themselves:

Oj.default_options = my = { mode: :strict }
require "rabl"

Oj.default_options != my

Oj has its share of responsibility on that, however, IMHO the bug comes from rabl.

I'd love to fix it by using MultiJson, yet I came across one of your commits removing it (9bfb1a4).

Another option may be to have a condition in the format_json method, but I'm not a big fan..

@nesquena
Copy link
Owner

Thanks for the report, I agree this is an issue. However, I am unclear of the cleanest way to solve it. I'll give it some thought 🤔 . MultiJson was removed just to streamline and remove dependencies. If anyone wants to submit a PR that they think fixes this relatively cleanly, I'd be happy to review, consider, and merge as well.

@BuonOmo
Copy link
Author

BuonOmo commented Sep 27, 2021

@nesquena well:

  • MultiJson is more downloaded than Rack or Rails (https://rubygems.org/stats) hence there are great chances one already depends to multi_json in its Gemfile,
  • It is only 1.2k LOC (rabl is 1.7k for instance),
  • On my computer, repositories have in average 1 dependency that itself depends on MultiJson:
    `rg multi_json **/Gemfile.lock -c`.split.map { _1[/\d+\z/].to_i - 1 }.sum / `fd Gemfile.lock | wc -l`.to_i

Hence I'd be incline to add it back 🙂

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