Skip to content

Remove j alias for ERB::Util.json_escape #3578

Merged
merged 1 commit into from Aug 21, 2012

7 participants

@amatsuda
Ruby on Rails member
amatsuda commented Nov 9, 2011

I noticed that this newly added j method https://github.com/rails/rails/blob/6e87281b5f/actionpack/lib/action_view/helpers/javascript_helper.rb#L37
overwrites existing another j method
https://github.com/rails/rails/blob/6e87281b5f/activesupport/lib/active_support/core_ext/string/output_safety.rb#L58
in ActionView context.

FYI: It seems this global j is called in other contexts. https://github.com/flori/json/blob/b50b1bdeae/lib/json/common.rb#L395

Attached a patch that removes j alias for json_escape because this j is no longer available for app developers, and not used inside the framework (maybe those who are using output_safety.rb out of the Rails box would be affected... but who does?).

@josevalim
Ruby on Rails member

While I agree it is confusing having two methods called j, j is also a module function for ERB::Util. And it is common for ERB::Util have aliases based on the escape method: http://www.ruby-doc.org/stdlib-1.9.2/libdoc/erb/rdoc/ERB/Util.html

It also freaks me out that some methods are called json_escape and others escape_javascript (I always get confused). It also annoys me they are in different files. Maybe the best solution would be:

1) Move escape_javascript to ERB::Util as well (or move all to AV::Helpers::EscapeHelper)
2) Alias escape_javascript to js
3) Deprecate j in all cases

But I am not sure. What do you think? /cc @jeremy

@jeremy
Ruby on Rails member
jeremy commented Jan 5, 2012

j mirrors h and is meant to only work in views. Global Kernel#j seems terrible: https://github.com/flori/json/blob/b50b1bdeae/lib/json/common.rb#L395

+1 to consolidating the implementations, in any case.

@jfirebaugh

My preference would be to consolidate in EscapeHelper, alias escape_javascript to js, and alias json_escape to j (with the much more useful definition provided in #6094).

@rapind
rapind commented Jun 21, 2012

+1

@dlee
dlee commented Aug 14, 2012

If j is changing to js and another j is taking its place, the documentation has to be eye popping since it'll surely be confusing.

I would personally recommend leaving j aliased to escape_javascript, since it has meant escape_javascript for ages.

@rafaelfranca rafaelfranca merged commit d65a15d into rails:master Aug 21, 2012
@amatsuda amatsuda deleted the amatsuda:remove_j_alias_for_json_escape branch Aug 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.