Remove .auto_register! method and add finer-grained registration config to component_dirs

[timriley]

This PR continues in the direction established with the rich component_dirs config and removes the Container.auto_register! method. It ensures that any configurability provided by this method is now available within the per-directory config that can be accessed when calling config.component_dirs.add.

This PR contains BREAKING CHANGES:

• Users of .auto_register! will need to switch to adding a component_dir and configuring it as required.
• Anyone subclassing Dry::System::Loader or providing a replacement loader will need to convert their copies to use class methods, with each one accepting a component as their first argument

More details below.

Rationale

There are two reasons to remove .auto_register!:

The primary reason is that it provides a way of “side-loading” a container with components outside of the main container lifecycle, which is to configure component_dirs up front, and then rely on either lazy-loading components or finalizing the container. With the design changes made in #155, the container now assumes that all possible component directory sources are configured within component_dirs, which .auto_register! sidesteps.

The secondary reason is that .auto_register! supports additional configuration around its registration behavior (when called with a block) that is not available for up-front configured component_dirs. If we want the component_dirs to be the only place for auto-registration configuration, we need to remove .auto_register! and provide equivalents for the additional settings that it provides.

New and expanded component_dir settings

This PR updates the settings available when adding component_dirs:

• auto_register can now either either be a boolean (which enables/disables auto-registration for all components in the directory) or a proc that accepts a component instance and should return a boolean or truthy/falsey value (which enables/disables auto-registration on a component-by-component basis, depending on the proc's return value). For details, see spec/integration/container/auto_registration/custom_auto_register_proc_spec.rb.
• a new loader setting allows a custom component loader class (see Dry::System::Loader) to be configured for all components in the directory. loader also remains a top-level setting for Dry::System::Container, with the top-level configured loader applying to all component directories that do not provide their own. See spec/integration/container/auto_registration/custom_loader_spec.rb.
• a new memoize setting allows customising whether components are memoized when registered with the container. Like auto_register, then can either be a plain boolean (to apply to all components in the directory), or a proc that accepts a component and returns a boolean or truthy/falsey value (which enables/disables memoization on a component-by-component basis, depending on the proc's return value). See spec/integration/container/auto_registration/memoize_spec.rb.

These settings apply when both lazy-loading components, as well as when the auto-registrar runs in full during container finalization.

These settings replace those previously available when calling .auto_register! with a block:

• auto_register with a proc replaces exclude (as an inversion, so e.g. previous configs returning true to exclude a component from auto-registration must now return false)
• loader replaces instance (the same responsibilities, just provided via reusing an existing concept within dry-system)
• memoize continues to serve the same function, though with the added flexibility of using a proc to return a per-component value (which was not the case for the auto_register!-based version of this setting)

This means that any users of .auto_register! with custom configuration will be able to upgrade to this new arrangement while preserving their configured behavior.

Details

• Container.auto_register! is deleted, along with all related specs. Thank you for your service, little method.
• Config::ComponentDir holds the 2 new setting definitions: loader and memoize
• In order for the top-level loader config to be available to each ComponentDir, these are now initialized with access to the entire container instead of just the container's root. This gives them access to the container's loader if there isn't a loader explicitly configured their directory
• ComponentDir also provides a new #component_options, which returns a hash of all the relevant options for each Component: this includes the three config values detailed above: auto_register, loader, and memoize
• Component gets a new .new_from_component_dir factory method, which extracts some of the logic from .locate (introduced in the previous PR), and is now used by the AutoRegistrar, which it already has an instance of each ComponentDir as it walks the directories and registers each component. This method uses the above-mentioned #component_options method to provide the relevant options to each initialized component. This helps ensure consistency in behaviour around Component creation across both the lazy-loading and container finalizing pathways.
• Now that Component has those options, it provides new #auto_register? and #memoize? predicate methods that apply the setting logic described above (i.e. respecting a plain bool or calling a proc with itself)
• Container.load_local_component (used when lazy loading) now determines whether or not to memoize the registration based on the component's #memoize? (this duplicates what AutoRegistrar does, perhaps signals the potential to extract a plain old Registrar component that could be used in both places)
• Loader is now implemented using class methods only, and is no longer initialized per-component. This never really needed instance-level state, and making this static will reduce the number of object instantiations by half during container finalization
• Container.component is now a private method, since (as of the previous PR), AutoRegistrar is no longer using it

Next steps

I need to verify this and the preceding PR in a real app. In order to do this, I need to update the hanami/hanami core to use this PR's branch of dry-system and configure component_dirs as appropriate. I plan to get this done this week.

Lastly, dry-rails will need to be updated to configure component_dirs instead of calling .auto_register!, which it does now.

Once both of these are done, we should merge these dry-system PRs and coordinate new 0.x.0 releases of both dry-rails and dry-system.

[solnic]

@timriley is this OK that it's gonna silently skip require when the component's file doesn't exist? Maybe it should raise an error in such case?

[solnic]

ps. I know it used to work like that but it could be an opportunity to improve it

[timriley]

@solnic I'm happy to reconsider this! Perhaps in a follow-up PR?

[solnic]

@timriley sounds good! It caught my attention because I remember that it can be really daunting to track down a configuration issue when dry-system is silently doing nothing. I'm sure you know what I' talking about. This also reminds me we need a debug mode with lots of logging. I'm happy to take care of this!

[solnic]

@timriley #156 👈🏻 I assume this will be addressed as a nice side-effect, right?

[timriley]

@solnic Regarding #156, I don't think this will be fixed by this PR. The scenario described in #156 revolves around registering within bootable components, which this PR doesn't touch at all.

I do wonder whether we could do away with that "lifecycle container" concept at the same time as we e.g. remove use from bootable components. The special nature of that environment does make things harder to understand than it needs to be, I think.

Aside from your couple of questions, are you happy with this PR?

[solnic]

@solnic Regarding #156, I don't think this will be fixed by this PR. The scenario described in #156 revolves around registering within bootable components, which this PR doesn't touch at all.

Ah ok thanks!

I do wonder whether we could do away with that "lifecycle container" concept at the same time as we e.g. remove use from bootable components. The special nature of that environment does make things harder to understand than it needs to be, I think.

I would love to remove it and I 100% agree with what you wrote here. I just didn't have a better idea at the time.

Aside from your couple of questions, are you happy with this PR?

Just accepted it 🙂 👏🏻 🙇🏻

[flash-gordon]

mb we should add a check for ruby version since sorting isn't required since ruby 3 https://bugs.ruby-lang.org/issues/8709 Ideally, it'll be a check around method definition

[flash-gordon]

it'd be better to "normalize" this option by mapping booleans to const functions, that is true -> -> _ { true }, false -> -> _ { false }. So that you won't need to check if it responds to :call

[timriley]

The tricky thing with this is that the auto_register option may come from in-file magic comments, which will be booleans as a result of its simple built-in coercions.

If we didn't support the magic comments, then the Config::ComponentDir class would be a good place to normalise these values, but because it would be the only place this value is configured.

Given this, do you think we should still try to do it? I like the outcome you're suggesting, but this arrangement means Component itself is really the only feasible place to normalise the value, and I feel like any work to do that here would probably wind up being worse/more verbose than what's currently in place. What do you think?

[timriley]

Happy to continue this conversation and find a nice way to do this in a follow-up PR, btw! But I think I might go ahead and merge this so we can begin our testing from master sooner rather than later :)

[timriley]

@flash-gordon Actually, here's what I've done just to tidy up the behaviour before considering further options:

      # @api private
      def auto_register?
        callable_option?(options[:auto_register])
      end

      # @api private
      def memoize?
        callable_option?(options[:memoize])
      end

      private

      def callable_option?(value)
        if value.respond_to?(:call)
          !!value.call(self)
        else
          !!value
        end
      end

[flash-gordon]

same here

[flash-gordon]

👏 👏 👏
finally got time to read it through 😅 My plan is to test my projects against master once it's merged. I'll be able to provide more feedback then. I don't think it's gonna be more than "I made it work!"