gelf-rb
gelf-rb copied to clipboard
graylog-labs
Remove opportunistic loading of `yajl/json_gem`
Requiring yajl/json_gem causes JSON.parse, JSON.generate, #to_json, and other built-in methods to be overridden with the faster YAJL implementation. Faster is, as a rule, better so this seems like a positive change overall. However, because this change extends outside of gelf-rb and impacts any place JSON is parsed or generated using the standard library interface, it can have surprising and unexpected consequences.
When I updated gelf-rb to version 3.1.0, parts of my app started to behave differently. Specifically, U+2028 and U+2029 characters stopped being escaped within JSON strings. For a Rails app, which already overrides the standard library interface, this can be a problem [1]. (This will only impact folks who use YAJL without require 'yajl/json_gem'.)
To remedy this, yajl/json_gem is no longer required by default. This makes the optimization of JSON parsing the responsibility of the app/script/person requiring gelf-rb.
[1] http://timelessrepo.com/json-isnt-a-javascript-subset
@AlekSi does this seem like a change useful to the project? Right now, we maintain an empty file to make sure yajl/json_gem isn't loaded when graylog-rb is required. 😅
I have no idea, I stopped maintaining this gem (and using it, and using Ruby) years ago.
Well, perhaps it would be a good idea to merge the change? Does anyone else have the ability to merge this in? @AlekSi ? That would be nice. "I don't know, I don't maintain this" is very unhelpful and results in a bunch of useless forks getting created for a project. The impact of this change is pretty significant and I think merging this would make 3.1.x usable for a wider audience.
I don't have access to merge this change. I'm not a maintainer for a long time already. Sorry.
/cc @lennartkoopmann
Alexey,
Thanks. I did open up a separate issue asking who can maintain. 👌👍🏻
T
On Mar 5, 2020, at 09:15, Alexey Palazhchenko [email protected] wrote:
I don't have access to merge this change. I'm not a maintainer for a long time already. Sorry.
/cc @lennartkoopmann
— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.