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 yajl and json gem with multi JSON? #195

Open
iainbeeston opened this issue Nov 29, 2014 · 19 comments
Open

Replace yajl and json gem with multi JSON? #195

iainbeeston opened this issue Nov 29, 2014 · 19 comments
Labels
Milestone

Comments

@iainbeeston
Copy link
Contributor

Right now there is a bit of custom code for both yajl and json-gem, which both behave slightly differently. I'd like to start a discussion about what the future should be for that.

I'd suggest that either:

  1. We switch to using multi_json (which supports all manner of json parsers with a unified api), or
  2. We switch to only supporting json (like rails did) and simplify the code a bit

There is an argument that multi_json is deprecated and nowadays the right move is for everyone to use json-gem, but I can't help thinking people should be given the choice (and therefore multi json is better)

@RST-J
Copy link
Contributor

RST-J commented Nov 29, 2014

I think it would only be worth to keep multi_json if there is another argument than simply having the choice between different parsing libraries. Is there anything another parser does really different?
If they are all more or less the same and we gain performance by sticking to one (json) then I think performance is more important.

@hoxworth
Copy link
Contributor

The reasoning for allowing additional JSON libraries is that other JSON libraries are more performant in many cases. Oj, for example, usually shows parsing speed improvement of 50% or more.

Rails can do what they like as a framework, but as a library we shouldn't lock users into what we think is right or wrong.

@RST-J
Copy link
Contributor

RST-J commented Nov 29, 2014

In this case I also favor keeping multi_json.

@iainbeeston
Copy link
Contributor Author

But should we also aim to use multi_json by default? Instead of having code that explicitly targets yajl and json-gem?

@iainbeeston
Copy link
Contributor Author

(My hope is we can reduce the amount of code by relying on just one gem for parsing json)

@iainbeeston
Copy link
Contributor Author

Actually, this raises the whole broader question of whether we should be parsing json at all. What if in version 3 we didn't parse input and expect the user to parse for themself and give json schema a hash? Right now the input can be a url, a json text or a hash and we have to use heuristics to see which one we have, but generally the caller will know. Even worse, a string, a string url and a string containing json can all be valid json texts, that could be parsed or treated as (already parsed json) and we have no way of knowing which we have

@RST-J
Copy link
Contributor

RST-J commented Nov 30, 2014

But what about $refs? If json-schema receives a schema for which the references schemas are not yet registered, we end up with the need to parse JSON. And I don't think that a user should have to register all referenced schemas by hand, that would feel tedious.
If we let the user pass in some proc to parse JSON, this also complicates the interface and I would feel inconvenient with having to do this, I guess.

@iainbeeston
Copy link
Contributor Author

Sorry, you're right. I'm getting confused between this issue (using one json api everywhere) and another issue (making JSON::Schema.validate not have to guess the meaning of any strings that it receives for schemas or data, by parsing them as json or loading them as urls)=

@pd
Copy link
Contributor

pd commented Nov 30, 2014

multi_json is mostly dying, as can be seen by nearly all large projects slowing dropping their dependency on it. I rather liked the idea of multi_json, but so it goes. =)

I use oj on pretty much any ruby application I work on that touches JSON, but for oj, you don't need multi_json, cuz you can just call Oj.mimic_JSON and it will patch itself into place.

So, 👍 for a single require 'json' (preferably in tandem with dropping 1.8.x support, so there's no dependency necessary).

@pd pd added this to the v3 milestone Nov 30, 2014
@RST-J
Copy link
Contributor

RST-J commented Dec 2, 2014

Can we find a consensus? @hoxworth is Oj.mimic_JSON alternative enough?
I have no particular opinion on this except for performance. So if there is a way we could keep multi_json in and at the same time avoid the performance penalties referred to when using standard json then this would be fine for me.

@hoxworth
Copy link
Contributor

hoxworth commented Dec 2, 2014

I am in favor of reducing the amount of code, so moving to just multi-json is fine with me (if that's what we're planning on doing). Most of that code existed before multi-json was in widespread use, and wasn't ever properly refactored away.

@iainbeeston
Copy link
Contributor Author

I have to admit, I don’t really understand the general argument against multi_json. Apparently it’s slower than just using the underlying gems directly?=

@RST-J
Copy link
Contributor

RST-J commented Dec 2, 2014

I thought that I've read something about multi_json being slow(er). Ah, it is actually the question in the title from the link in your first post. Ok, then it is just a guess. Again a point where a benchmark would come in handy to probably outweigh the (to me) only argument against multi_json.

@iainbeeston
Copy link
Contributor Author

Ok, I'm starting to believe we should only support the json gem. Multi json is "deprecated", plus OJ and Yajl both have a json-gem compatible mode (Oj.mimic_json and require 'yajl/json_gem'). Seems like json-gem is the new "standard"

@RST-J
Copy link
Contributor

RST-J commented Dec 9, 2014

Is there any other major/serious JSON implementation we would rule out by this decision? If all serious alternatives have a compatibility mode then it seems fine to me.

@brancz
Copy link
Contributor

brancz commented Jun 10, 2015

In that case, only the code that specialises on yajl has to be removed if I am correct? I would do the changes if you can confirm that it is the consensus.

@iainbeeston
Copy link
Contributor Author

Yes, I think so
On Wed, 10 Jun 2015 at 7:46 pm, Frederic Branczyk notifications@github.com
wrote:

In that case, only the code that specialises on yajl has to be removed if
I am correct? I would do the changes if you can confirm that it is the
consensus.


Reply to this email directly or view it on GitHub
#195 (comment)
.

@brancz
Copy link
Contributor

brancz commented Jun 21, 2015

Nevermind, it seems like #206 already did the work, I looked at the wrong branch. I guess this issue can be closed?

@iainbeeston
Copy link
Contributor Author

Sorry, yes. It's schedule for v3.
On Sun, 21 Jun 2015 at 9:12 pm, Frederic Branczyk notifications@github.com
wrote:

Nevermind, it seems like #206
#206 already did
the work, I looked at the wrong branch. I guess this issue can be closed?


Reply to this email directly or view it on GitHub
#195 (comment)
.

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

No branches or pull requests

5 participants