Skip to content
This repository has been archived by the owner on Jun 30, 2018. It is now read-only.

Remove multi_json and replace with stdlib json #776

Closed
weynsee opened this issue Jun 30, 2013 · 11 comments
Closed

Remove multi_json and replace with stdlib json #776

weynsee opened this issue Jun 30, 2013 · 11 comments
Labels

Comments

@weynsee
Copy link
Contributor

weynsee commented Jun 30, 2013

With multi_json's future being a little murky and the standard lib json (in 1.9) performing much faster than multi_json, it might be time to remove the dependency on multi_json.

If there's interest in this I can submit a PR for this.

@weynsee
Copy link
Contributor Author

weynsee commented Jun 30, 2013

For 1.8 users, they would have to use the json gem.

@karmi
Copy link
Owner

karmi commented Jun 30, 2013

Let's first wait and see how MultiJson future will pan out. As noted in the original issue, MultiJson allows everybody to use their preferred library without changing the interface -- without an abstraction like that, Tire would have to provide something like this on its own.

@weynsee
Copy link
Contributor Author

weynsee commented Jul 1, 2013

Sounds good. Do you want to close this issue or keep it open for now?

@phoet
Copy link
Contributor

phoet commented Jul 5, 2013

👍 for this. we only had trouble using MultiJson anyways. always had to do a lot of monkey-patching for MultiJson and tire...

@karmi
Copy link
Owner

karmi commented Jul 5, 2013

@phoet That's surprising -- would you be able to dig out some concrete issue/backtraces/whatever? I've never had any problem. That said, adding an abstraction for handling JSON is technically trivial, just laborious. I'd like to wait a bit and see -- I've seen these kind of problems being solved simply by waiting until dust settles.

@phoet
Copy link
Contributor

phoet commented Jul 5, 2013

@karmi remember all those rails json loading, symbolizing keys issues?

after that we switched to OJ as a json parser (really fast, reduced our response-times by half) and MultiJson was overriding all default OJ settings, so that we had to patch it to use our default settings. this seems to be resolved right now.

another thing was our dependency to couch_potato, which does not use MultiJson, so we basically had 2 JSON config points etc...

@phoet
Copy link
Contributor

phoet commented Jul 5, 2013

a little offside, but are there any plans to drop 1.8.7 support?

@karmi
Copy link
Owner

karmi commented Jul 5, 2013

Definitely not on current Tire .)

@karmi
Copy link
Owner

karmi commented Jul 5, 2013

after that we switched to OJ as a json parser (really fast, reduced our response-times by half)

Yeah, saw the performance implications in the linked issue and I'm definitely not happy about it. Still, we need to somehow make the JSON lib pluggable, which should be the sole responsibility and reason for MultiJson. Hopefully the situation will improve. I still believe abstractions a la MultiJson and Faraday are quite useful, since as a library author, you don't have to provide extension points yourself.

another thing was our dependency to couch_potato, which does not use MultiJson, so we basically had 2 JSON config points etc...

Ah, yeah, that sounds really painful...

@karmi
Copy link
Owner

karmi commented Jan 15, 2014

Seems like https://github.com/intridea/multi_json is live and kicking?

For the record, https://github.com/elasticsearch/elasticsearch-ruby uses MultiJson and so far no issues have been reported. elasticsearch-ruby has modular serializer though, so you can pass whatever JSON library, including custom wrappers to it: http://rubydoc.info/gems/elasticsearch-transport/#Serializer_Implementations

Closing this?

@phoet
Copy link
Contributor

phoet commented Jan 15, 2014

👍

@karmi karmi closed this as completed Jan 16, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants