Solidus: RFC: Change *_decorator pattern

[kennyadsl]

The Solidus suggested way to extend Ruby classes coming from core and extensions into users' applications is by creating files that end with _decorator.rb into the /app folder of the main application.

Despite this is not mandatory and users can change this pattern, this is suggested by us with the install generator that pushes this code into config/application.rb

    config.to_prepare do
      # Load application's model / class decorators
      Dir.glob(File.join(File.dirname(__FILE__), "../app/**/*_decorator*.rb")) do |c|
        Rails.configuration.cache_classes ? require(c) : load(c)
      end

I think this is not a good pattern and we should stop using/promoting it:

• it is not clear
• it is not really related to the decorator pattern that (I think) it was originally referring to
• it may conflict with naming used by other gems, Draper for example.

I hope this issue helps to answer these questions:

• Does the community agree that we should change it?
• What is a good pattern/naming convention instead? Some proposals have been already done (https://github.com/solidusio/solidus/pull/3008#issuecomment-448587039)
• Does switching to the new pattern breaks backward compatibility or is it something we should do in a major release (maybe 3.0)?

[jacobherrington]

We can deprecate the *_decorator.rb pattern now with a warning and add support for using *_override.rb (or whatever), then remove support for the old pattern in 4.0? Is that too long term, or does it make sense?

That's assuming that the 3.0 release (https://github.com/solidusio/solidus/milestone/16) isn't too far off.

[elia]

This is relevant doc from Rails: https://guides.rubyonrails.org/engines.html#a-note-on-decorators-and-loading-code

Not sure if this habit came first on Rails or on Spree, but it might be good to adhere to whatever standard Rails has (except for extremely good reasons).

[kennyadsl]

@elia didn't know that, great find! I think that could be a good reason to keep it as is then.

[jacobherrington]

If we keep it the way it is, let's document why we do that here: https://github.com/solidusio/solidus/blob/master/guides/source/developers/extensions/decorators.html.md

[elia]

@kennyadsl the merit goes to @bitberryru, also noting that those aren't actual decorators. I agree that article should be linked in the docs. Maybe would be good to fix Rails docs too, if we come up with a good proposal.

[bitberry-dev]

Of course, I support it :)

The name should be changed, it will not break anything but this will be consistent with the principle of least surprise/astonishment. It is ruby way for sure.

Maybe would be good to fix Rails docs too

It would be great 👍🏻👍🏻👍🏻

[mdesantis]

What about this part of Rails docs:

Using Class#class_eval is great for simple adjustments, but for more complex class modifications, you might want to consider using ActiveSupport::Concern. ActiveSupport::Concern manages load order of interlinked dependent modules and classes at run time allowing you to significantly modularize your code.

Working on Solidus I found having to use class_eval pretty awkward, instead using ActiveSupport::Concern sounds better to me (more flexible, more idiomatic). The cons is that it requires a simple but huge change: move Solidus models logic to concerns. But it would also give us the opportunity to reorganize and modularize models logic.

Could it worth the effort?

[kennyadsl]

Not sure to understand. We could move core things into concerns but still need some mechanism to change Solidus core functionalities into applications, which is where "decorators" are used. Could you please explain this more in depth @mdesantis ?

[mdesantis]

@kennyadsl well, actually I didn't try anything practical, I'm checking what they mean in the Rails docs just now. Basically, if in Solidus you move all the class logic to a main concern, e.g.:

module Spree
  class PromotionRule < Spree::Base
    include Spree::Models::PromotionRule

    def hello
      'Hello from Solidus'
    end
  end
end

Then in your application you could have:

module Spree
  class PromotionRule < Spree::Base
    include Spree::Models::PromotionRule

    def hello
      "#{super} and from my app"
    end
  end
end

But it's actually not a "decoration", it's a "redefinition + include all the previous logic": you are actually redefining Spree::PromotionRule. So, in order to use this technique, also extensions should implement all the logics into concerns, and then in your app you'll have to write something like:

module Spree
  class PromotionRule < Spree::Base
    include Spree::Models::PromotionRule
    include Spree::SomeExtension::Models::PromotionRule

    def hello
      "#{super} and from my app"
    end
  end
end

Moreover, you'll have to do the same for every class defined by Solidus (e.g. in the example above if you want to customize Spree::Base, you have to move in Solidus core all the Spree::Base logic to a main concern and in your app redefine 'Spree::Base' + extend it).

I'm unsure whether is better or not: from one hand it makes your app classes more explicit about what they include, from the other hand it's more verbose than class_eval implementation. Any thoughts?

[bitberry-dev]

@mdesantis As far as I understand, you are trying to re-invent "prepend" :)

For my application, I use my own Spree::Extension module, which is very similar to ActiveSupport::Concern except one thing - my "concern" prepending, not including.

app/lib/spree/extension.rb

module Spree
  module Extension
    def prepend_features(base)
      super
      base.singleton_class.prepend const_get('ClassMethods') if const_defined?("ClassMethods")
      base.class_eval(&@_overrode_block) if instance_variable_defined?(:@_overrode_block)
    end

    def overrode(&block)
      raise MultipleIncludedBlocks if instance_variable_defined?(:@_overrode_block)
      @_overrode_block = block
    end

    def class_methods(&class_methods_module_definition)
      mod = const_defined?(:ClassMethods, false) ?
                const_get(:ClassMethods) :
                const_set(:ClassMethods, Module.new)

      mod.module_eval(&class_methods_module_definition)
    end

    # This method prepends extension to appropriate class (ex. SomeClassNameOverrideSomething to SomeClassName)
    def override!(delimiter = 'Override')
      overridable_class = Object.const_get(self.to_s.split(delimiter).first)
      overridable_class.prepend(self)
    end
  end
end

And typical override looks like
app/controllers/spree/checkout_controller_override_processing_offsite.rb

module Spree
  module CheckoutControllerOverrideProcessingOffsite
    extend Spree::Extension

    overrode do
      before_action :check_important_things, only: [:update]
    end

    private

    def completion_route
      return custom_path(...) if condition?
      super
    end

    def check_important_things
       # ...
    end

    override! # same as Spree::CheckoutController.prepend(self)
  end
end

As you see you can use super because your patch will be prepended, not included, it happens when you call override! inside it.

This allows you to extend any external classes and use inheritance features like you're declaring a child class.

[mdesantis]

@bitberryru you're right, I was re-inventing prepend :-) indeed I like your implementation, ideally I'd want something like that integrated into ActiveSupport::Concern, so to have at the same time the advantages of Concerns and of Prepends. There are various examples of it out there:

• https://gist.github.com/bcardarella/5735987
• https://github.com/DavyJonesLocker/ruby-easy_auth/blob/master/lib/easy_auth/active_support/concern.rb
• https://gitlab.com/gitlab-org/gitlab-ee/commit/f62067552441620527afeb03bbed26e4ab2da301

Anyway, great news on the Rails side: rails/rails#34946 has been merged, so now it's official that talking about decorators in this context it's not appropriate!

[kevinnio]

As of today, rails docs still read

6.1.1 A note on Decorators and Loading Code

:(

[kennyadsl]

Not sure why, but it's updated on master. 🤔

[tvdeyen]

I am 💯 for making this change as this always bugged me for above mentioned reasons 👏

But, I wholeheartedly disagree with calling them *_overrides.rb. These things are monkey patches. Please, let's call them what they are then (*_monkey_patch.rbs). Everybody knows what a monkey patch is. We even call them monkey patches every time we explain what these "decorators" are meant for. So, why another disguise?

Also, Deface, a still widely used gem in Solidus ecosystem, already uses the app/overrides folder.

[bitberry-dev]

Hi guys,

Finally, a beautiful autoloader in Rails now. Unfortunately, documentation lacks information on how to load decorators by using it. You might have come across some tips, such as using require, load, or require_dependency, though all of them are wrong or even harmful.

I’ve researched zeitwerk API and have found an excellent method called preload, which is suitable for decorators. By using it, you can select files which need to be loaded first and it will follow this during an initial setup and reloading. This is exactly what we need for decorators loading, because decorators are the part of the code that should be loaded first like gems.

Below you can find an example of my production code. You just add once your decorators in a preload list and everything will be working perfectly well. If changes are made, code will reboot correctly, zeitwerk will do all the job.

config.after_initialize do
   Rails.autoloaders.main.preload(Dir.glob(Rails.root.join('app/**/*_override*.rb')))
end

There are couple of moments. The current approach for loading decorators in solidus engines (https://github.com/solidusio/solidus_support/blob/master/lib/solidus_support/engine_extensions.rb#L36) calls require_dependency in to_prepare hook, that fires during every reboot (btw, what for?) and due to this ancestors chain breaks down

after reload

Decorators, that were barbarously (for zeitwerk) loaded with require_dependecy moved to the first place in a chain and this could lead to bugs. In my case as you can see Spree::CheckoutControllerDecorator defined in solidus_auth_devise was required in to_prepare hook and break ancestors chain which in result led to the wrong behaviour of completion_route method.

I suggest starting to use preload in engines, for example, declaring them in initializers before zeitwerk “takes over”.
P.S. In fact, it’s not obligatory to do it before that moment, but during “take over” zeitwerk is doing a setup; after the setup the following preload calls will instantly load an additional code; and before the setup preload simply adds the file in the preload list that will be loaded only on setup.

initializer :preload_solidus_<gem_name>_overrides, before: :let_zeitwerk_take_over do
    Rails.autoloaders.main.preload(Dir.glob('path_to_overrides/**/*_override*.rb')))
end

Such an approach will allow inject decorator into any part of the ancestors chain on application-level. Let’s say there are 2 different decorators from 2 different gems and we want to inject exactly between them. We can do it. This gives us so much flexibility, that we couldn’t even dream of before!

As an option, you can continue to load engine decorators as you did in the past (require, load, require_dependency), but then I would ask you to do it only once at any convenient time for you. Mostly importantly that after it I would be able to call zeitwerk’s preload.

Decorators need clearer practices and better name.

What do you think about this? Let's talk.

[kennyadsl]

@bitberryru thanks a lot for your insights, let's talk! 🙂

The call at https://github.com/solidusio/solidus_support/blob/master/lib/solidus_support/engine_extensions.rb#L36 is needed because those files are never referenced in the codebase (like any other class/module) and unless we don't require them "manually" on every app reload (with to_prepare) they are not loaded at all.

Please also consider that the approach taken is needed to support both Rails with and without Zeitwerk autoloader so I guess we can't use the preload feature, isn't it?

What about changing how you load decorators in your application in order to have them loaded with the "barbarously" (😆) way which I think would preserve the loading order as the first boot?

[elia]

@bitberryru I quickly checked the implementation of preload in zeitwerk, and basically it boils down to require, you know if zeitwerk will reload the decorators as well?

My initial though when I saw zeitwerk was that we could write a mapper that tries to load any foo_decorator when Foo is referenced, along with foo (although I'm not sure it's possible to load multiple files).

@kennyadsl if a better way to support reloading of decorators comes available through zeitwerk I think a config switch would make a lot of sense and solve many problems for those who choose that path. Would that be acceptable?

_Anyway I too think we should solve the to_prepare problem. I tried in the past (and without success so far) with different attempts, including using AS::Dependency.clear as the first to_prepare block and replacing it with an initial preload inside an initializer and the to_run block (which is fired only once per request, see also https://github.com/solidusio/solidus/pull/3474)._

[bitberry-dev]

@kennyadsl

is needed because those files are never referenced in the codebase (like any other class/module)

Yes, of course I understand that :) I asked this because at the application level you do not need to reload the gem's decorators because their code does not change.

Please also consider that the approach taken is needed to support both Rails with and without Zeitwerk autoloader so I guess we can't use the preload feature, isn't it?

Well, in general we can support both. But also keep in mind that classic mode is discouraged for new applications. I don’t think it's worth focusing on classic autoloader.

What about changing how you load decorators in your application in order to have them loaded with the "barbarously" (😆) way which I think would preserve the loading order as the first boot?

Yes, for some time it will still work while zeitwerk allows it. But for zeitwerk it is barbarous way :)) because it is assumed that the application code should be loaded with an autolader.
And I have no difficulty fixing this bug, I just suggest a better approach as I think in working with zeitwerk.

@elia

I quickly checked the implementation of preload in zeitwerk, and basically it boils down to require, you know if zeitwerk will reload the decorators as well?

Yep, it will and it will always preserve load order.

Anyway I too think we should solve the to_prepare problem. I tried in the past (and without success so far) with different attempts, including using AS::Dependency.clear as the first to_prepare block and replacing it with an initial preload inside an initializer and the to_run block

I began to think that only I had various problems with the classic autoloader and the "classic" approach to loading decorators. But I'm not alone 👍

[kennyadsl]

I think what we are doing is also what Zeitwerk expects in this scenario. Please, take a look at https://github.com/solidusio/solidus_support/pull/43.

Anyway, I totally understand that we need to solve this issue. This could be via code or via some good documentation around how to create decorators and where to place them in order to be compliant with the new Rails autoloader. Would you be available for a chat in Slack in the next days?

[bitberry-dev]

I think what we are doing is also what Zeitwerk expects in this scenario. Please, take a look at solidusio/solidus_support#43.

Yep, it expects and all works because Zeitwerk overrides Kernel's require where handles such a case. For example if you try to change require to load all your decorators will load two times (in my applications each decorator tracks this case and raises an exception, but this may not be noticeable in other applications or you will see notices about scope overwriting)

In short, I think a more idiomatic and preferred way to load code is to use autoloader api. require and require_dependency is something like bypassing api and such things in my opinion should be 🔫 eliminated as proposed here https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#require-dependency

Would you be available for a chat in Slack in the next days?

Yes, I’ll come now

[elia]

Last week me and @spaghetticode finally got around to leveraging Zeitwerk to do things in the right way™ and this PR is the resulting work:

https://github.com/solidusio/solidus_support/pull/60