Namespace Conflicts

[skukx]

From: https://github.com/solidusio/solidus_support/blob/master/lib/solidus_support/engine_extensions.rb#L12-L14

# /app/decorators
# @see https://github.com/solidusio/solidus_support/blob/master/lib/solidus_support/engine_extensions.rb#L60
solidus_decorators_root.glob('*') do |decorators_folder|
  config.autoload_paths += [decorators_folder]
end

This line will take each folder inside /app/decorators and add it to the autoloads path.

For example:

/app/decorators/my_gem
/app/decorators/my_other_gem

Will be added like

config.autoload_paths += '/app/decorators/my_gem'
config.autoload_paths += '/app/decorators/my_other_gem'

Could this be problematic? The reason I raise this question is for two reasons. The first is that this somewhat abandons ruby conventions.

Expected

# /app/decorators/my_gem/spree/product_decorator.rb
module MyGem
  module Spree
    module ProductDecorator
    end
  end
end

Actual
The above raises an error due to zeiwerk expecting the file to be defined like:

# /app/decorators/my_gem/spree/product_decorator.rb
module Spree
  module ProductDecorator
  end
end

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:

/app/decorators/my_gem/spree/product_decorator.rb
/app/decorators/my_other_gem/spree/product_decorator.rb

The autoloader sees this as

/spree/product_decorator.rb
/spree/product_decorator.rb

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 from my_gem first but my_other_gem will not have its own Spree::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.

[skukx]

Re-creation

From: https://github.com/skukx/solidus_demo1/blob/master/app/decorators/solidus_demo1/spree/product_decorator.rb

# /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

From: https://github.com/skukx/solidus_demo2/blob/master/app/decorators/solidus_demo2/spree/product_decorator.rb

# /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'

# ...

Expected output

rails-console> Spree::Product.new
Called From SolidusDemo1
Called From SolidusDemo2
=> <Spree::Product 0x0000>

Actual

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

[jarednorman]

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.

[aldesantis]

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).

[skukx]

@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?

[aldesantis]

Sure thing: https://edgeguides.solidus.io/customization/customizing-the-core.

And yes, I think we can close.

[skukx]

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.

[aldesantis]

@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.