Replace occurences of alias_method_chain with their Module#prepend counterpart - #19413
Conversation
|
Thanks for this, looks like some tests are failing. I will be putting some more work on it. |
|
Should we follow through with a deprecation on |
|
Thanks for review! I've pushed some updated parts. @kaspth it would be great to deprecate I tried it and I also tried to fix the failing test, but I had no success yet. I will spend some more time on it tomorrow 💛 |
|
Well, the question was more aimed at any Core Team members that might have happened to listen in. But yes, do look more into it 😄 Kasper
|
|
The tests are failing here because of a SystemStackError. After looking into it, I highly suspect it being a ruby bug and I've found this issue which sounds like our problem. If that's the case, this particular modification will have to wait again I guess. |
|
@fbernier you are right. We can't just replace @kirs Regarding module ActiveSupport
module CoreExt
module ToJsonWithActiveSupportEncoder
def to_json(options = nil)
if options.is_a?(::JSON::State)
# Called from JSON.{generate,dump}, forward it to JSON gem's to_json
super
else
# to_json is being invoked directly, use ActiveSupport's encoder
ActiveSupport::JSON.encode(self, options)
end
end
end
end
end
[Object, Array, FalseClass, Float, Hash, Integer, NilClass, String, TrueClass, Enumerable].reverse_each do |klass|
klass.prepend ActiveSupport::CoreExt::ToJsonWithActiveSupportEncoder
endThe key here is to use |
|
Thanks @yuki24! I updated all the code. @kaspth @rafaelfranca @fbernier what do you think? |
There was a problem hiding this comment.
I think this module should have the ActiveSupport namespace.
👍 |
|
But lets add the deprecation in a different PR. |
Thanks @fbernier for suggestion! <3 At this moment we can use Module#prepend in all all cases except of Range because of the bug [1] in MRI 2.2 [1] https://bugs.ruby-lang.org/issues/10847
|
@rafaelfranca updated with the namespaced module. I hope it's ready for merge 🚀 |
|
I commented but don't need to change. I'll do. |
Replace occurences of alias_method_chain with their Module#prepend counterpart
…Hash(ActiveSupport::ToJsonWithActiveSupportEncoder)#to_json for Ruby 2.8.0 This pull request addresses failures at https://buildkite.com/rails/rails/builds/70219#79d96882-6c51-4854-8cab-28f50ac8bca1 According to https://bugs.ruby-lang.org/issues/16973 This is an expected change in Ruby. These failures has been addressed by changing the order of prepend as suggested. ```diff % git diff diff --git a/activesupport/test/json/encoding_test.rb b/activesupport/test/json/encoding_test.rb index 30a3b8e..1328041bf7 100644 --- a/activesupport/test/json/encoding_test.rb +++ b/activesupport/test/json/encoding_test.rb @@ -186,6 +186,8 @@ def test_hash_should_pass_encoding_options_to_children_in_to_json country: "UK" } } + p person.method(:to_json) + pp person.class.ancestors json = person.to_json only: [:address, :city] assert_equal(%({"address":{"city":"London"}}), json) @@ -287,6 +289,8 @@ def test_array_to_json_should_not_keep_options_around f.bar = "world" array = [f, { "foo" => "other_foo", "test" => "other_test" }] + p array.method(:to_json) + pp array.class.ancestors assert_equal([{ "foo" => "hello", "bar" => "world" }, { "foo" => "other_foo", "test" => "other_test" }], ActiveSupport::JSON.decode(array.to_json)) end % ``` * Ruby 2.8.0 without this fix uses `Array(JSON::Ext::Generator::GeneratorMethods::Array)#to_json`, which should use `Array(ActiveSupport::ToJsonWithActiveSupportEncoder)#to_json` ``` % bin/test test/json/encoding_test.rb -n test_array_to_json_should_not_keep_options_around Run options: -n test_array_to_json_should_not_keep_options_around --seed 33311 [Array, JSON::Ext::Generator::GeneratorMethods::Array, ActiveSupport::ToJsonWithActiveSupportEncoder, Enumerable, ActiveSupport::ToJsonWithActiveSupportEncoder, Object, JSON::Ext::Generator::GeneratorMethods::Object, ActiveSupport::Tryable, Kernel, BasicObject] F Failure: TestJSONEncoding#test_array_to_json_should_not_keep_options_around [/Users/yahonda/src/github.com/rails/rails/activesupport/test/json/encoding_test.rb:294]: --- expected +++ actual @@ -1 +1 @@ -[{"foo"=>"hello", "bar"=>"world"}, {"foo"=>"other_foo", "test"=>"other_test"}] +["#<TestJSONEncoding::CustomWithOptions:0xXXXXXX>", {"foo"=>"other_foo", "test"=>"other_test"}] bin/test test/json/encoding_test.rb:286 Finished in 0.015486s, 64.5745 runs/s, 64.5745 assertions/s. 1 runs, 1 assertions, 1 failures, 0 errors, 0 skips % ``` * Ruby 2.8.0 with this fix uses `Array(ActiveSupport::ToJsonWithActiveSupportEncoder)#to_json` ``` % bin/test test/json/encoding_test.rb -n test_array_to_json_should_not_keep_options_around Run options: -n test_array_to_json_should_not_keep_options_around --seed 12193 [ActiveSupport::ToJsonWithActiveSupportEncoder, Array, JSON::Ext::Generator::GeneratorMethods::Array, ActiveSupport::ToJsonWithActiveSupportEncoder, Enumerable, ActiveSupport::ToJsonWithActiveSupportEncoder, Object, JSON::Ext::Generator::GeneratorMethods::Object, ActiveSupport::Tryable, Kernel, BasicObject] . Finished in 0.008070s, 123.9157 runs/s, 123.9157 assertions/s. 1 runs, 1 assertions, 0 failures, 0 errors, 0 skips % ``` * Ruby 2.8.0 without this fix uses `Hash(JSON::Ext::Generator::GeneratorMethods::Hash)#to_json`, which should use `Hash(ActiveSupport::ToJsonWithActiveSupportEncoder)#to_json` ``` % bin/test test/json/encoding_test.rb -n test_hash_should_pass_encoding_options_to_children_in_to_json Run options: -n test_hash_should_pass_encoding_options_to_children_in_to_json --seed 18064 [Hash, JSON::Ext::Generator::GeneratorMethods::Hash, ActiveSupport::ToJsonWithActiveSupportEncoder, Enumerable, ActiveSupport::ToJsonWithActiveSupportEncoder, Object, JSON::Ext::Generator::GeneratorMethods::Object, ActiveSupport::Tryable, Kernel, BasicObject] F Failure: TestJSONEncoding#test_hash_should_pass_encoding_options_to_children_in_to_json [/Users/yahonda/src/github.com/rails/rails/activesupport/test/json/encoding_test.rb:193]: --- expected +++ actual @@ -1 +1 @@ -"{\"address\":{\"city\":\"London\"}}" +"{\"name\":\"John\",\"address\":{\"city\":\"London\",\"country\":\"UK\"}}" bin/test test/json/encoding_test.rb:181 Finished in 0.015009s, 66.6267 runs/s, 66.6267 assertions/s. 1 runs, 1 assertions, 1 failures, 0 errors, 0 skips % ``` * Ruby 2.8.0 with this fix uses `Hash(ActiveSupport::ToJsonWithActiveSupportEncoder)#to_json` ``` % bin/test test/json/encoding_test.rb -n test_hash_should_pass_encoding_options_to_children_in_to_json Run options: -n test_hash_should_pass_encoding_options_to_children_in_to_json --seed 56794 [ActiveSupport::ToJsonWithActiveSupportEncoder, Hash, JSON::Ext::Generator::GeneratorMethods::Hash, ActiveSupport::ToJsonWithActiveSupportEncoder, Enumerable, ActiveSupport::ToJsonWithActiveSupportEncoder, Object, JSON::Ext::Generator::GeneratorMethods::Object, ActiveSupport::Tryable, Kernel, BasicObject] . Finished in 0.007434s, 134.5171 runs/s, 134.5171 assertions/s. 1 runs, 1 assertions, 0 failures, 0 errors, 0 skips % ``` Refer ruby/ruby#3181 ruby/ruby#2936 https://bugs.ruby-lang.org/issues/9573 rails#19413
Earlier @fbernier proposed a cool idea of replacing
alias_method_chainwithModule#prependin #17394.I guess it's now time to merge it 🚀
/cc @rafaelfranca