Closed skukx closed 4 years ago
# /app/decorators/solidus_demo1/spree/product_decorator.rb
module Spree
module ProductDecorator
def initialize(*_args)
puts 'Called from SolidusDemo1'
super
end
::Spree::Product.prepend(self)
end
end
# /app/decorators/solidus_demo2/spree/product_decorator.rb
module Spree
module ProductDecorator
def initialize(*_args)
puts 'Called from SolidusDemo2'
super
end
::Spree::Product.prepend(self)
end
end
The creating a new solidus app from scratch
# Gemfile
# ...
gem 'solidus_demo1', github: 'skukx/solidus_demo1'
gem 'solidus_demo2', github: 'skukx/solidus_demo2'
# ...
rails-console> Spree::Product.new
Called From SolidusDemo1
Called From SolidusDemo2
=> <Spree::Product 0x0000>
rails-console> Spree::Product.new
Called From SolidusDemo1
=> <Spree::Product 0x0000>
rails-console> Spree::Product.ancestors
=> [Spree::ProductDecorator, Spree::Product::Scopes, Spree::Product]
Notice the conflicting namespace.
This may be a feature but it should be documented how these decorators are loaded to avoid confusion. For those experiencing this issue may be better off formatting files like:
# /app/decorators/models/solidus_demo1/spree/product_decorator.rb
module SolidusDemo1
module Spree
module ProductDecorator
end
end
end
This way we avoid naming conflicts at the top level
Yeah, I'm of the opinion that in general extensions should exclusively use their own namespaces and that the decorator loading code should be set up to support that.
I agree 100%, decorators should be namespaced under their extension's name or under the app's name when they're defined in the app itself. It's something we've started doing in extensions and it should become the standard (the pattern has already been included in the new Solidus guides).
@aldesantis Should I close this issue then? If so can we post a link to those docs in case anyone finds this issue in the future?
Sure thing: https://edgeguides.solidus.io/customization/customizing-the-core.
And yes, I think we can close.
Hey @aldesantis
Looking at the example from https://edgeguides.solidus.io/customization/customizing-the-core#using-decorators.
# app/decorators/awesome_store/spree/product/add_global_hidden_flag.rb
module AwesomeStore
module Spree
module Product
module AddGlobalHiddenFlag
def available?
ENV['MAKE_PRODUCTS_UNAVAILABLE'] == 'true' && super
end
::Spree::Product.prepend self
end
end
end
end
This is only true for rails applications. However, for extensions this will cause a zeiwerk error because of the way extensions are loaded. The error raised with be something like:
Expected /app/decorators/awesome_store/spree/product/add_global_hidden_flag.rb to define `Spree::Product::AddGlobalHiddenFlag` but got `AwesomeStore::Spree::Product::AddGlobalHiddenFlag`.
This is due to how extension decorators are loaded.
@skukx decorators should be prefixed with the type of class they decorate, e.g. app/decorators/controllers/my_store
. This makes it easier to find all controller decorators. I see we’re not following this pattern in the guides, so I’ll make sure to update the examples.
From: https://github.com/solidusio/solidus_support/blob/master/lib/solidus_support/engine_extensions.rb#L12-L14
This line will take each folder inside
/app/decorators
and add it to the autoloads path.For example:
Will be added like
Could this be problematic? The reason I raise this question is for two reasons. The first is that this somewhat abandons ruby conventions.
Expected
Actual The above raises an error due to zeiwerk expecting the file to be defined like:
Notice that the namespace doesn't exactly reflect the actual depth. I don't believe this to be a big issue and am ok with it. However this brings me to the second point which is name space conflicts.
Back to a directory structure of:
The autoloader sees this as
When the autoloader looks to load these files you now have two exact files to be loaded. The problem is that
Spree::ProductDecorator
will be loaded frommy_gem
first butmy_other_gem
will not have its ownSpree::ProductDecorator
loaded because it thinks it has already been loaded.Instead of adding the sub folders of
/app/decorators
to the autoload path. We should just have the/app/decorators
path be the only autoloaded.