fxn/zeitwerk

Questions regarding Zeitwork reloading and rails engines

timdiggins opened this issue · 8 comments

timdiggins commented

Thanks very much for the amazing work on zeitwerk (and also the documentation both in the README here and within rails - superb).

I've asked some questions in Stack Overflow: https://stackoverflow.com/questions/74576577/questions-about-reloading-and-engines-in-rails-with-zeitwerk-autoloader but then I realised maybe an issue was the better place to ask (and document this). So I'm duplicating here - apologies if that's bad form.

Background:

I am getting some reloading errors in development when switching to Zeitwerk for a rails 6.0 application that uses an engine (thredded). I'm also a developer on thredded, so want to understand this fully before committing any apparent fixes:

I've already read:

First Question (separation of autoloading and engines): Are engines autoloaded in the same zeitwerk instance/loader as the main app, or are they somehow loaded separately? That is to say does wrapping some code with Rails.application.reloader.to_prepare do ensure that code is run before both the main app and the engine are reloaded.

Second Question (engine code reloading): Are engine's constants reloaded when the main app reloads? (my understanding is yes).

Third Question (configuring engines): Currently Thredded's docs suggest that the configuration of Thredded happens in an initializer -- e.g. Thredded.some_configuration_option = value - but I think that would get wiped away with autoloading? So therefore probably needs to be wrapped with (I think) Rails.application.reloader.to_prepare do (but this isn't what e.g. Devise recommends (see https://github.com/heartcombo/devise/blob/main/lib/generators/templates/devise.rb) and seems to conflict with #143). What have I misunderstood here?

Fourth Question (all these to_prepares): Can someone explain or point me to docs that clarify the lifecycle difference between:

  • Rails.application.reloader.to_prepare do (and is the block run at least once even when it is eager loaded in production with reloading on)
  • Rails.application.config.to_prepare do
  • SomeEngine::Engine.config.to_prepare do
timdiggins commented

(following up from StackOverflow).

So you've quickly confirmed that

(1) Yes, the main autoloader manages both the application and all engines.
(2) Your understanding is correct.
(3) The Thredded constant is not reloadable, it is defined in lib/thredded.rb, which is a top-level file required. Only what is in the engines app directory is autoloaded and reloaded (by default, don't see anything special configured). (4) The existing config.to_prepare seems good to me and is what you want to use. What problem do you have exactly? –

And I've realized that the problem is probably in Thredded::Workgroup that overrides thredded in a pre-zeitwerk way and needs to define "overrides" just like the main app does.

So as per https://guides.rubyonrails.org/engines.html#overriding-models-and-controllers, I need to do something in Thredded::Workgroup::Engine analogous to what happens in application.rb in the main app - something like

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

I'll try this, but not sure what the equivalent of Rails.autoloaders.main is within the engine...

fxn commented

Let's continue here! Then when we find out the issue I can write an answer in SO for the archives.

Thanks very much for you kind of words about the documentation. There's a lot of love in those docs and I truly appreciate it.

Let me first say that these lines indicate this engine understands autoloading/reloading and that the inclusion of Thredded::UserExtender is done correctly.

Rails.application.main is the loader that manages both the parent application and all the engines. It does not matter if you access that from the engine, conceptually, the loader has no notion of engine vs application, it is all a set of autoload paths without more semantics.

Would it be possible to have a minimal application that reproduces the error you are facing? If not, I'd try to understand via questions.

timdiggins commented

Thanks! I'll see if I can create a minimal application, but in the first place I'm trying to fix Thredded::Workgroup (separate engine: https://github.com/thredded/thredded-workgroup) to make it zeitwerk compatible as I think this may be the source of the issues.

In terms of the way that Thredded::Workgroup manages its overrides : I can't see a way to do the following from the Thredded::Workgroup engine:

overrides = root.join("app/overrides")
Rails.autoloaders.main.ignore(overrides)

because Rails.autoloaders.main.ignore(overrides) won't work because there is no application at the point when the Engine is loaded AFAICT.
However I realized that it's not critical to have the overrides in the app path - I can just put them in lib/overrides, and then I don't need to ignore them (let me know if this is not correct).

fxn commented

That engine configures autoload paths by hand here. This is unnecessary because Rails does that for you automatically. Also, putting lib in there is bad, and it is doing nothing because a walkthrough seems to say all the code in lib is required when the entrypoint is loaded. I don't think this is going to fix anything, but I believe it can be deleted.

Can you explain to me the overrides? What is that engine overriding, and where are the original files?

timdiggins commented

Ah - thanks very much for looking into Thredded::Workgroup and for the heads up on the autoloading : yes makes total sense.

Question: Even if the /lib directory is all required, if it is (erroneously) autoloaded, would that make it be reloaded in development (and if it is reloaded, bad things might happen). I'm going to test what happens when I remove that. (Unfortunately there are other issues in the repo to fix also).

In terms of the overrides, there are quite a few, e.g.: https://github.com/thredded/thredded-workgroup/blob/main/app/view_models/thredded/topic_view.rb - I think I can see how to make this compatible with zeitwerk, namely:

  • using Thredded::TopicView.class_eval rather than reopening / redeclaring class Thredded::TopicView
  • removing the now unnecessary require_dependency
  • ensuring it is ignored by autoloading (either with an ignore call or moving it to e.g. lib/overrides)
  • ensuring it is loaded on reload (as per https://guides.rubyonrails.org/engines.html#overriding-models-and-controllers but in the Engine)

I think this will all go relatively smoothly, but I have to fix some (unrelated) issues around url helpers in the engine first to get a clean test suite (broken by a recent commit in Thredded). I'll let you know how I get on.

timdiggins commented

According to my rough and ready tests, this has now fixed the reloading problem in the main app that was using thredded workgroup and thredded engines! 🎉 Thanks very much @fxn for the help.

It would be slightly cleaner if there was a way to include the overrides within app (e.g. in app/overrides) if there's a way to call Rails.autoloaders.main.ignore(overrides) that will work from within an engine.rb. But feel free to close this if you think that using lib/overrides is actually adequate.

fxn commented

Yeah, you can put the overrides under app and ignore the directory if you prefer.

If you'd like it that way, the autoloaders are instantiated very early, when the application instance object is created. And they are global objects and public interface. So, the engine can put that configuration in an initializer (untested):

initializer "let the main autolaoder ignore overrides" do
  overrides = root.join("app/overrides")
  Rails.autoloaders.main.ignore(overrides)
end

Zeitwerk API is designed so that configuration can be incrementally built from anywhere while the application boots. So, this is all OK.

timdiggins commented

Adding it as an initializer like that works (verified with Rails.autoloaders.main.log!).
Nice! thanks @fxn