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 (Fix autoload issue. Replace require/load with require_dependency. #3008 (comment))
• 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 #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 solidusio/solidus_support#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:

solidusio/solidus_support#60

[waiting-for-dev]

At this point, Rails encourage in its guides a way to monkey patch from the application:

https://guides.rubyonrails.org/engines.html#overriding-models-and-controllers

Engine models and controllers can be reopened by the parent application to extend or decorate them.

Overrides may be organized in a dedicated directory app/overrides that is preloaded in a to_prepare callback.

In zeitwerk mode you'd do this:

# config/application.rb
module MyApp
  class Application < Rails::Application
    # ...

    overrides = "#{Rails.root}/app/overrides"
    Rails.autoloaders.main.ignore(overrides)
    config.to_prepare do
      Dir.glob("#{overrides}/**/*_override.rb").each do |override|
        load override
      end
    end
  end
end

and in classic mode this:

# config/application.rb
module MyApp
  class Application < Rails::Application
    # ...

    config.to_prepare do
      Dir.glob("#{Rails.root}/app/overrides/**/*_override.rb").each do |override|
        require_dependency override
      end
    end
  end
end

As we see, it uses the word overrides to name the files that contain the monkey patches.

Another issue is how you monkey patch those classes. In Ruby and Rails, you have several options. To name a few:

• class_eval
• Reopen a class
• include a module
• prepend a module
• Use an ActiveSupport::Concern
• Use prependers.

The Rails guides talk about two of them: class_eval and ActiveSupport::Concern, but you really can use whatever method you prefer.

Existing an official way to implement that pattern, I'd say we don't need to do anything besides linking to the Rails guides. Even more, taking into account that our goal should be to provide actual extensibility through the injection of services or a similar pattern.

However, we have to add deface into the equation. deface is not a dependency of solidus, but we're suggesting its usage on the guides. On top of that, it's a dependency on solidus_auth_devise, but we're using it to override a single file.

The problem with deface is that it uses the same directory that Rails suggest for the monkey patching files: app/overrides/. In fact, it has a funny consequence. Adding deface as a dependency, both directly or transitively through solidus_auth_devise, makes it unnecessary to add the code that Rails suggest to application.rb. Just add the _overrides.rb files into app/overrides, and it does the trick.

We did some tests in this repository https://github.com/waiting-for-dev/rails_overrides with a bare rails app:

• We create an engine within the app.
• That engine has a series of "models" that we'll expect to be overridden from the main app.
• All of them define a method foo which returns a string "From engine", except for the _with_include.rb one that doesn't define it.
• In the main app, within the app/overrides folder and prepending with _override.rb, we override the engine models
• We redefine the same foo method, but in this case, it returns "From app"
• We configure application.rb as it's detailed on the guides.
• We add tests to check that all the models get indeed overridden.
• We add a root endpoint to test if code reloading works manually.

The repository has several branches which test different combinations of rails, rails + deface, rails + solidus and rails + solidus_auth_devise, with both Zeitwerk and the classic loader. In all cases, the test suite is green, and code reloading works. When deface or solidus_auth_devise are added to the Gemfile, as we said, there's no need to do anything with application.rb.

There's also a with_deface_alongside_overrides that adds two overrides (following the two proposed ways) in the same app/overrides folder. They are loaded on the call to the /bar endpoint, and we can see it doesn't interfere with the model monkey patching, as both tests and the code reloading on / keep working.

The "help" given by deface should not be seen as a nice side effect. To begin with, it breaks the expectations that a developer has following the Rails guides. On top of that, mixing deface overrides with generic monkey patching is not clean. We should try to update deface upstream to use a different directory or make it configurable, and then we change it from Solidus. Another option is to deprecate deface on Solidus altogether. It's only a matter of stop suggesting it on the guides and manually overriding the single view changed in solidus_auth_devise. deface comes with its disadvantages, so not requiring it through solidus_auth_devise can be seen as an independently good outcome.

[aldesantis]

I agree that we should adopt the Rails convention for overriding engines instead of trying to reinvent the wheel. Perhaps we can have a preferred way of doing the overrides (concerns?), because extensive use of class_eval leads to a lot of problems and should be discouraged, while the Rails guides take a neutral approach towards it.

As for Deface, I don't think getting rid of it will be as simple as removing it from solidus_auth_devise unfortunately (although we should definitely do that). While we strongly discourage extensions from attempting to override anything in the frontend, Deface is still the only (more or less) maintainable way to override things in the backend, and a lot of extensions use it precisely for that purpose. The only alternative is replacing entire backend views, which obviously doesn't work when you install multiple extensions that all override the same view, and makes upgrading Solidus a nightmare.

We've talked a lot about how to get rid of Deface for the backend, and the final solution will most likely require providing more extension hooks for the backend UI (similar to what we already do for menu items) and introducing ViewComponent (so that it's easier to override or configure tiny pieces of the backend in isolation).

For the time being, I think that we should allow Deface to accept the overrides path as a configuration option, and push to deprecate and then remove the default setting of app/overrides, so that it doesn't clash with Rails overrides. A couple of us are also Deface maintainers, and I'm sure we can work with our Spree friends to get this done, since it's in the best interest of both platforms.