Make env file loading a clearer step when subclassing app

[timriley]

This makes more sense than loading it as a side effect of preparing the settings store.

Yes, the settings are popualted from env vars, but that’s not the only thing in an app that may rely upon them, so loading dotenv as early as possible makes more sense.

To address this, we:

1. Move dotenv loading into Hanami::App (in particular the private .load_dotenv) method
2. Call .load_dotenv as soon as App is subclassed, making the populated ENV available as early as possible (this would be useful for e.g. referring to ENV vars inside the App subclass body)
3. Rename Hanami::Settings::DotenvStore to EnvStore
4. Create a new set of integration tests to cover the various .env files that can be loaded

[timriley]

FWIW, a lot of these tests now overlap with the ones I added in the (also open) #1186. Once these are both merged, I'll remove any redundant tests (leaving this file as the canonical set of tests exercising the .env file loading behaviour).

In the meantime, I figured it made sense to let each PR stand on its own.

[solnic]

Why was this needed? It doesn't sound like a good idea to load file contents in an inherited hook. The only thing that should depend on dotenv behavior is the settings object. I'm having trouble understanding why we need to load it so early (which makes the app coupled to dotenv).

[timriley]

Thanks for checking in on this, @solnic :)

There's a couple of different things that led me to this change.

1. We need to plug the gap left by the change of settings to an ordinary component

Firstly, in #1186, we've made settings as much an ordinary component as possible, with it now fully managed by its own provider, and no longer made available on Slice via a dedicated accessor:

Settings accessor is removed from Slice

[...] 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"]

I think this is a good change on balance, but it does mean one that that we used to be able to do is no longer possible, which is accessing the settings in the body of the app class. For example, from an earlier version of our app template:

module AppPrototype
  class Application < Hanami::Application
    config.sessions = :cookie, {
      key: "app_prototype.session",
      secret: settings.session_secret,
      expire_after: 60 * 60 * 24 * 365 # 1 year
    }

    config.logger = {
      level: :debug,
      stream: settings.log_to_stdout ? $stdout : "log/#{Hanami.env}.log"
    }
  end
end

Because settings is being turned into an ordinary container component, it will no longer be available at such an early point as the evaluation of the app class, like we see above. Instead, it will only be available once the prepare phase has run (by which point the app/slice config is also frozen).

Now this feels like a reasonable change to me, and IMO it helpfully focuses the role of the settings object towards settings that the user wants to make within their "userspace" in the Hanami app (i.e. everything that happens after the framework configuration).

Notably, this change to the settings component is essential if we want to allow it to autoload constants from the rest of the app, which was the whole purpose of that PR in the first place. We can't have a super-early-loading setting object and access to autoloading at the same time; we have to pick one.

So after this change, however, we are leaving a gap in terms of how the user might want to configure their app. If there's some app settings that aren't entirely static, the only real approach that remains for them is to use ENV vars directly.

So by priming the ENV from .env files as early as possible, it should allow our users to use values loaded from those files in configuring their app as required.

Really, what it means is that ENV is available and populated as expected wherever the user wants it, which is what they would expect, especially given the expectations we set around how the settings interact with it.

This brings me to the next couple of reasons for this change here.

2. This isn't an actual change (turns out), just removal of obfuscation around dotenv loading

The first is that we were already loading dotenv very early in the app lifecycle: as soon as the configuration object is initialized, which is (can you believe it), run as part of the App.inherited hook — i.e. in exactly the same place we have it here in this PR.

Effectively, the only thing we're doing in this PR is making the dotenv loading more explicit, rather than having it be tucked away behind two other (fairly unrelated) layers. I think this is a positive change for maintainability.

3. What we're doing here is consistent with dotenv-rails

In fact, this is exactly the same spot as dotenv-rails uses to load itself as part of Rails' initialization:

dotenv is initialized in your Rails app during the before_configuration callback, which is fired when the Application constant is defined in config/application.rb with class Application < Rails::Application. If you need it to be initialized sooner, you can manually call Dotenv::Railtie.load.

So... it seems like we're actually following a reasonable approach here? Clearly it's worked well enough for dotenv-rails to keep it.

4. There are other potential uses of ENV coming up

Over in #1189 (my PR to add conditional slice loading), I'm adding support for using the HANAMI_LOAD_SLICES and HANAMI_SKIP_SLICES env vars as a convenience for populating the App's config.slices.load_slices and config.slices.skip_slices config.

For users to be able to work with these env vars just like any others on their system, we'll want to make sure that the .env files are loaded at the right time (i.e. before the configuration object is initialized). This is what we achieve with this PR here. In fact, this dotenv change here was originally part of #1189, but I decided to split it out for easier reviewing (which, given this discussion, I think was a good idea!).

5. This isn't an undesirable coupling, and it is also opt-out

You're not wrong to say this is a "coupling" of the app to dotenv, but it's only a coupling insofar as we're also coupled to e.g. dry-configurable or dry-system: we're choosing to use a library to provide useful behaviour.

In the case of dotenv, we're allowing users to opt into or out of this behaviour by virtue of including the dotenv gem in the new app's generated Gemfile (which I've just recently fixed in hanami/cli#29) instead of keeping it in our gemspec here. If the user removes the line from their Gemfile, we'll gracefully skip the dotenv loading.

---

I didn't expect this to get so long, but there you go! How does this sit with you now, @solnic?

[solnic]

@timriley thank you, this makes sense, and you're awesome 😄 I'm just a bit reluctant whenever I see complex logic added to inherited hook as it gave us a lot of pain in rom-rb in its early days. One idea I had to avoid it here was to make it very explicit by having an extra line in the generated class skeleton, like "load_dotenv" or something, just to make it clear that it's happening. We can of course stick to what you did and see how it goes. It's reassuring that this is how dotenv-rails works. I appreciate your thoughtful explanation ❤️

[jodosha]

@timriley LGTM. 👍
Left a couple of comments for potential enhancements.

[jodosha]

@timriley I want to double check this with you: what's the Public API to access env variables?

In H1 we had Hanami::Env, reasons are written here: https://lucaguidi.com/2016/12/27/isolate-global-state/

---

I'm asking because we could populate Hanami::Env, instead of ENV, by using Dotenv.parse(*dotenv_files), which returns a Hash.

[timriley]

@jodosha It's a good question. This PR certainly doesn't have the ambition of providing such an interface — its job is just to populate ENV as we've always done, but just in a clearer way.

As for whether we should provide a first party, "safer" interface to ENV, my current feeling is that it's probably not worthwhile it at this point: we should be encouraging our users to take advantage of our settings components to access relevant values from the env. Cases where that is not possible (like some of the scenarios I described in my comment above) are hopefully relatively minor and not the kind of thing that would be close to the actual working business logic of Hanami apps.

What do you think?

[timriley]

Thanks again @solnic for the initial prompt and @jodosha for checking all of this out 😄

@solnic as for your idea for having load_dotenv or similar be baked out into our generated config/app.rb class, I think that's a reasonable possibility to keep alive, but in the spirit of keeping as much as possible not baked out, I do think it's worth the experiment of the automatic approach as we have here (especially since it's mostly just a rearrangement of behaviour that was already in place).

A related thing that I would like to consider in the post-2.0 world is some kind of well-defined, extensible structure for the overall framework app boot process. In this world, we could have "load dotenv" as one of the pre-defined steps in the boot, which a user could just explicitly disable if they didn't want it.

Anyway, I'll merge this one in now and we can collectively keep an eye out for any issues. That said, my feel is that we'll be fine to bring this through to the 2.0 releases :)

[solnic]

A related thing that I would like to consider in the post-2.0 world is some kind of well-defined, extensible structure for the overall framework app boot process. In this world, we could have "load dotenv" as one of the pre-defined steps in the boot, which a user could just explicitly disable if they didn't want it.

Yes I've had that idea for a long time but I thought about making it a feature in dry-system. It'd allow you to define a Unit of Work for the booting process where steps can have dependencies and then it's resolved, sorted and executed in the right order based on your configuration. When this process is hardcoded, you lose a lot of flexibility.

[timriley]

Load Dotenv separately from settings store