1
0
Fork 0
mirror of https://github.com/drapergem/draper synced 2023-03-27 23:21:17 -04:00

Only Draper::Decorator types should delegate === to object. (#720)

Draper::Decorater provides the object method and we should delegate === to this
method only for these types.

Other classes could respond_to?(:object) so using is_a? clarifies not just any
class implementing object but only classes that are a Draper::Decorator.

Besides this, another major reason is is_a? is much faster than respond_to?
in ActiveRecord models because of the number of methods AR provides and the
layers of delegation. There is a cache specifically for respond_to? [1] because
it's so slow.

Even with this cache, some of our pages are 10% faster by using the is_a? change
from this patch.

The original implementation [2] used the method source, which was later renamed
to object.  The tests seem to indicate the intention was to always have
this === delegation to source/object for decorated objects... which is what
Drapper::Decorator types are.

Test changes:
* verify decorated instances of Product and instances derived from
decorated instances of Product are === Product.
* verify unrelated decorated instances and non-decorators that
happen to implement object are not === Product.

[1] https://github.com/rails/rails/blob/4-2-stable/activemodel/lib/active_model/attribute_methods.rb#L343-L351
[2] ca116154f3
This commit is contained in:
Joe Rafaniello 2017-03-31 15:51:10 -04:00 committed by Cliff Braton
parent bcbfed6f3f
commit e79ae14518
2 changed files with 10 additions and 4 deletions

View file

@ -86,7 +86,7 @@ module Draper
# #
# @return [Boolean] # @return [Boolean]
def ===(other) def ===(other)
super || (other.respond_to?(:object) && super(other.object)) super || (other.is_a?(Draper::Decorator) && super(other.object))
end end
end end

View file

@ -109,19 +109,25 @@ module Draper
end end
it "is true for a decorated instance" do it "is true for a decorated instance" do
decorator = double(object: Product.new) decorator = Product.new.decorate
expect(Product === decorator).to be_truthy expect(Product === decorator).to be_truthy
end end
it "is true for a decorated derived instance" do it "is true for a decorated derived instance" do
decorator = double(object: Class.new(Product).new) decorator = Class.new(Product).new.decorate
expect(Product === decorator).to be_truthy expect(Product === decorator).to be_truthy
end end
it "is false for a decorated unrelated instance" do it "is false for a decorated unrelated instance" do
decorator = double(object: Model.new) decorator = Other.new.decorate
expect(Product === decorator).to be_falsey
end
it "is false for a non-decorator which happens to respond to object" do
decorator = double(object: Product.new)
expect(Product === decorator).to be_falsey expect(Product === decorator).to be_falsey
end end