Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create an autoloader per slice; allow access to autoloaded constants from settings #1186

Merged
merged 11 commits into from Jul 27, 2022

Conversation

timriley
Copy link
Member

@timriley timriley commented Jul 20, 2022

This PR started out the seemingly simple goal of making it so you can access autoloaded constants from within a config/settings.rb file, which turned out to require a few deeper changes. In the end, I think these changes further improved our slices, making them even more self-contained (more on those details below!).

Anyway, now when you have e.g. a types module defined in another (appropriately named) file, you can access it inside the settings class like so:

# config/settings.rb

module MyApp
  class Settings < Hanami::Settings
    # MyApp::Types can come from lib/types.rb or app/types.rb
    setting :some_flag, constructor: Types::Params::Bool
  end
end

The above is for the app's settings. For settings within a slice, you access autoloaded constants from within the slice or the parent app:

# slices/main/config/settings.rb

module Main
  class Settings < Hanami::Settings

  # Slice-specific Main::Types can come from slices/main/types.rb
  setting :some_flag, constructor: Types::Params::Bool

  # Or if you want to use app-level types, MyApp::Types can come from
  # lib/types.rb or app/types.rb like the example above
  setting :another_flag, constructor: MyApp::Types::Params::Bool
end

In order to make this possible, this required a few internal changes:

Every slice now has its own Zeitwerk::Loader autoloader instance

This is a change from our previous arrangement of a single loader for the whole app, shared by each slice.

With this arrangement, I ran into a tricky limitation about Zeitwerk::Loader: that you can only call #setup on it just once. In a world where things are progressively loaded (i.e. the app, followed by each slice), this limitation becomes an issue:

  • We want to load the settings as part of each app/slice's prepare phase (so we can offer early feedback about failures to load),
  • If we also want the settings to have access to autoloaded constants, we need to call #setup on the loader before the settings are loaded,
  • But we can't do this, because as soon as we call #setup once, we'll block any subsequent slices from adding their paths to the autoloader.

To avoid this problem altogether, I've made it so that each slice has its own autoloader instance. I did consider this approach back when I was introducing Zeitwerk in the first place, but steered away from it because I thought it best to follow Rails' implementation first, which only has a single "main" autoloader for the whole application.

However, the reality is that we're different from Rails, and with our slices intended to be as self-contained as possible, it makes sense for each to have its own autoloader. The big benefit from this approach for our use case above is that we can now more tightly control when the per-slice autoloader is setup, which allows us to do this before we load the per-slice settings (more on this below).

Slice settings loading is moved to a new ensure_prepared step

Previously, we loaded the settings before all of our prepare steps, but now we do it very last, in a new ensure_prepared method.

This gives the chance of the rest of the prepare steps to run — including the autoloader setup — and it means that the code inside the settings file is evaluated in a way that is the same as every other ordinary component within the slice, i.e. once the slice has been prepared.

Slices are prepared after autoloader is #setup

In a related change, we've also moved the loading of slices further back in the list of prepare steps. This means the slice parent's autoloader is already setup before any of its child slices are loaded. More specifically, in the case of top-level slices in an Hanami app, it means that the App's autoloader is already setup, so slices can access app-managed constants (which we demonstrate in the second of the code examples at the top of this PR).

All details of settings loading have been moved into Hanami::Providers::Settings

Another theme of this change is to make the settings component as ordinary and self-contained as possible: the Slice class shouldn't be cluttered with details about settings loading.

This change wasn't critical for achieving the functional goals described up top, but I believe it's been a useful refactoring and will make settings easier to maintain.

So now, all aspects of settings loading are kept in the Hanami::Providers::Settings class, leaving the integration with the slice as a simple one-liner in .prepare_container_providers:

Providers::Settings.register_with_slice(self)

Settings accessor is removed from Slice

As evidenced through the provider setup above, settings is now effectively a standalone component, just one that we expect to be present in most Hanami apps (like our inflector and logger, etc.).

In this spirit, we want to encourage people to consider it as a regular resolvable (and injectable!) component rather than something hard-coded into the framework, so we've removed the dedicated Slice.settings accessor, which means users will now always access the settings in a single, consistent way, through component resolution: slice["settings"]

Full range of settings use cases have been documented through integration tests

To round out the settings changes, I've expanded our tests for how they might be used, now spread across a few files in spec/new_integration/settings/:

access_to_constants_spec.rb
loading_from_env_spec.rb
settings_component_loading_spec.rb
using_types_spec.rb

Hopefully these will serve as both useful regression tests as well as introductions to the overall functionality :)

Removed dry-types gemspec dependency

Lastly, in working on these tests, I noticed that we had dry-types as a runtime dependency in our .gemspec file, even though it's not used anywhere in Hanami's own lib/ code.

As such, I've moved it out of the gemspec and into the project's Gemfile. Since we're generating a lib/[app_name]/types.rb via the hanami new CLI command, we'll need to make sure we update our generator to place dry-types into the generated project's Gemfile too.

We don’t actually require dry-types for any core framework components.
This will allow us to entirely manage autoloader setup within the slice, which is useful for any aspects of the slice’s boot process that may need to depend on autoloadable code.
This makes it possible for child slices to reference any code that may be autoloaded by the parent.
@timriley timriley changed the title Create an autoloader per slice; allow access to autoloader constants from settings Create an autoloader per slice; allow access to autoloaded constants from settings Jul 20, 2022
@timriley timriley force-pushed the autoloader-per-slice-and-load-settings-after-autoloader branch from ddecf02 to 9bb849d Compare July 20, 2022 05:27
This keeps the details of the settings loading inside the provider class, simplifying Slice.

By loading the settings after the autoloader is set up, we also ensure that constants made available by the autoloader can be referenced within the Settings class (like e.g. a Types module). This takes away a potential hitch for users, and makes the settings class much more like any ordinary class in terms of access to the rest of the application code.
@timriley timriley force-pushed the autoloader-per-slice-and-load-settings-after-autoloader branch from 9bb849d to 16c7f87 Compare July 20, 2022 05:57
This can be accessed via `app["settings"]` or `slice["settings"]` like other ordinary components.
@timriley
Copy link
Member Author

Include some light refactoring for good measure
@timriley timriley force-pushed the autoloader-per-slice-and-load-settings-after-autoloader branch from a59f6a7 to d0bac45 Compare July 20, 2022 11:24
It’s no longer conditional, so it doesn’t make sense tucked away like this anymore
@timriley timriley marked this pull request as draft July 20, 2022 12:07
@timriley timriley self-assigned this Jul 21, 2022
@timriley timriley requested a review from jodosha July 21, 2022 02:02
@timriley timriley added this to the v2.0.0 milestone Jul 21, 2022
@timriley timriley marked this pull request as ready for review July 21, 2022 02:03
Copy link
Member

@jodosha jodosha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timriley Great enhancement! 👏
I love the parity of one container/autoloader per slice.

@timriley timriley merged commit bad567c into main Jul 27, 2022
@timriley timriley deleted the autoloader-per-slice-and-load-settings-after-autoloader branch July 27, 2022 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants