Push decorators to autoload path using config.autoload_paths

[kennyadsl]

This PR fixes a bug when decorators in app/decorators weren't properly reloaded when changing any other classes of the application.

I verified this by opening a rails console on an extension's dummy app using this module and running:

# Suppose we have a decorator called 
#   Spree::DecoratedCoreClassDecorator 
#
# that extends (via prepend) a core class called
#   Spree::DecoratedCoreClass
#
# the decorator is defined at:
#   extension_root/app/decorators/models/spree/decorated_core_class_decorator.rb

[1] pry(main)> Spree::DecoratedCoreClass.ancestors
=> [Spree::DecoratedCoreClassDecorator,
 Spree::DecoratedCoreClass,
 ActiveSupport::Dependencies::ZeitwerkIntegration::RequireDependency,
 ActiveSupport::ToJsonWithActiveSupportEncoder,
 Object,
 PP::ObjectMixin,
 FriendlyId::ObjectUtils,
 JSON::Ext::Generator::GeneratorMethods::Object,
 ActiveSupport::Tryable,
 ActiveSupport::Dependencies::Loadable,
 Kernel,
 BasicObject]

[2] pry(main)> reload!
Reloading...
=> true

[3] pry(main)> Spree::DecoratedCoreClass.ancestors
=> [Spree::DecoratedCoreClass,
 ActiveSupport::Dependencies::ZeitwerkIntegration::RequireDependency,
 ActiveSupport::ToJsonWithActiveSupportEncoder,
 Object,
 PP::ObjectMixin,
 FriendlyId::ObjectUtils,
 JSON::Ext::Generator::GeneratorMethods::Object,
 ActiveSupport::Tryable,
 ActiveSupport::Dependencies::Loadable,
 Kernel,
 BasicObject]

With this change, decorators are being correctly reloaded when some code in the application changes.

Apparently, there's no need to push root_folders into Zeitwerk directly via Rails.autoloaders.main.push_dir(decorators_folder) and we can rely on config.autoload_paths.

Another thing that I can't get yet is: we are calling require_dependency on all decorators with load_solidus_decorators_from(solidus_decorators_root) in the engine to_prepare method. This means that it's requiring the file again and again at each code reload. But if it was requiring the decorators at each code reload why we couldn't see it within the ancestors? Even if it wasn't autoloaded we should have been able to see it at that point.

[aldesantis]

@kennyadsl thanks, this makes sense! Not sure why the to_prepare block wasn't properly reloading the decorators, but I suspect it may have something to do with the order of operations that happen during the reload.

[fxn]

This patch seems good to me, and in Rails config.autoload_paths is the public interface so that is better also in that sense 👍.

The reason why the previous version did not work is a bit convoluted, the root cause is that to_prepare runs too late to configure the autoloader, let me explain.

The to_prepare block runs after autoloading has been set up during application boot (in the finalizer), so changes in the autoloader configuration are too late for the first run, and the require_dependency call made the behavior confusing.

Let's imagine we have app/decorators/models/foo.rb. When the app boots, the autoloader knows nothing about app/decorators/models because it was not in the autoload paths yet. But require_dependency was requiring the file, so the Foo constant is in memory.

On reload, the constant Foo is not unloaded because it was not actually autoloaded.

On reload, Zeitwerk visits app/decorators/models because it is now in the root paths, but since the constant Foo is already defined, the file foo.rb is ignored. (This emulates how require works in Ruby, it loads the first file it finds, and if there other ones later in the load path, they are ignored).

On the other hand, require is idempotent, so foo.rb was not interpreted again.

Note also that to_prepare was pushing dirs on each reload, which is suspicious. The method is idempotent, but you see that the natural thing is to configure them once. Which is what this patch does.

[kennyadsl]

@fxn well, thanks a ton for jumping on this one and for the accurate explanation, now everything seems pretty clear.

🙏 🙏 🙏

[aldesantis]

Thanks for chiming in @fxn! @kennyadsl let's merge 🚀

[fxn]

I have learned that app/decorators is meant to contain only true decorators and no code in there is autoloaded. In that case there might be a more polished solution still, that I believe would fit better this use case.

First, when an app has no intention to autoload from a directory, it is better to tell Zeitwerk to ignore it. On one hand, it is good programming, it is intentional. On the other hand, Zeitwerk is going to visit the directory otherwise and set autoloads matching its files and subdirectories when such autoloads should not be defined.

Unfortunately Rails does not have API to do that as of this writing, and classic does not have this feature, but reaching for the Zeitwerk instance if in zeitwerk mode is OK.

The second observation is that we are using require_dependency for a file that does not autoload anything, but in development mode we want to interpret those files manually an arbitrary number of times. The idiomatic way to do that in Ruby is load.

So, this is untested, but this is the patch I have in mind:

diff --git a/lib/solidus_support/engine_extensions.rb b/lib/solidus_support/engine_extensions.rb
index 6bfa5c4..97c3daa 100644
--- a/lib/solidus_support/engine_extensions.rb
+++ b/lib/solidus_support/engine_extensions.rb
@@ -9,8 +9,8 @@ module SolidusSupport
       engine.extend ClassMethods

       engine.class_eval do
-        solidus_decorators_root.glob('*') do |decorators_folder|
-          config.autoload_paths += [decorators_folder]
+        if Rails.respond_to?(:autoloaders) && Rails.autoloaders.zeitwerk_enabled?
+          Rails.autoladers.main.ignore(solidus_decorators_root)
         end

         config.to_prepare(&method(:activate))
@@ -33,7 +33,7 @@ module SolidusSupport
       # existing classes.
       def load_solidus_decorators_from(path)
         path.glob('**/*.rb') do |decorator_path|
-          require_dependency(decorator_path)
+          load decorator_path
         end
       end

[kennyadsl]

@fxn first thing, thanks for taking the time for this. I have a couple of questions/concerns:

Ignoring files

It makes sense but when calling

if Rails.respond_to?(:autoloaders) && Rails.autoloaders.zeitwerk_enabled?

it looks like the autoloader is not yet loaded in Rails. I get this error:

railties-6.0.2.2/lib/rails.rb:48:in `configuration': undefined method `config' for nil:NilClass

What about wrapping that code into a config.before_eager_load do? Is that a good place to be sure we have everything set?

require_depencency vs load

I'm giving load a try but if I change something in the decorator and manually reload! the app via rails console, it doesn't take the changes to the decorator and I have to restart the application to seee them. Is that supposed to happen?

[fxn]

Ahhhh, I see.

So the files are expected to define the constant that match their name, and you want to be able to cleanly reload. They do not decorate freely, but app/decorators/models/foo_decorator.rb is expected/required to be written like

module FooDecorator
end

Foo.prepend(FooDecorator)

Correct?

If that is the case, please forget my comment, it makes sense to have the decorators in the autoload paths because they follow the naming conventions and you do want the autoloader to manage them.