Classic autoloader support

[gmcgibbon]

Classic autoload reloading seems to be broken in the dummy app (and apps that use the classic autoloader). You can reproduce the error with the following:

• cd test/dummy
• CLASSIC_AUTOLOADER=1 bin/rails c
• reload!

Reloading...
~/src/github.com/Shopify/maintenance_tasks/app/models/maintenance_tasks/task.rb:62:in `csv_collection': uninitialized constant CsvCollectionBuilder (NameError)

Adding a require_dependency to app/models/maintenance_tasks/task.rb fixes the problem, but our support for non-Zeitwerk applications is unclear. Should we support them, or not? If not, I suggest we remove the ENV var to boot in classic mode. Also, keep in mind Rails 7 will not include the classic autoloader as Zeitwerk has been the default for quite awhile now.

[adrianna-chang-shopify]

our support for non-Zeitwerk applications is unclear

We currently ensure that we're compatible with the classic autoloader by eager loading the app and requiring all task files in the engine:

maintenance_tasks/lib/maintenance_tasks/engine.rb

Lines 11 to 13 in 05f03c4

	initializer "eager_load_for_classic_autoloader" do
	eager_load! unless Rails.autoloaders.zeitwerk_enabled?
	end

maintenance_tasks/lib/maintenance_tasks/engine.rb

Lines 21 to 26 in 05f03c4

	unless Rails.autoloaders.zeitwerk_enabled?
	tasks_module = MaintenanceTasks.tasks_module.underscore
	Dir["#{Rails.root}/app/tasks/#{tasks_module}/*.rb"].each do |file|
	require_dependency(file)
	end
	end

So I'd say that we have tried to support it up until this point 😄 That said, I'm not sure it's worth doing. As you said, Zeitwerk is the default in Rails 6 apps, and classic autoloading won't be supported soon, so I think it's fair game to enforce Zeitwerk. It also lets us get rid of the above code. I'd be OK to disable classic mode and call it out in the docs.

[etiennebarrie]

I think it was just a matter of qualifying CsvCollectionBuilder (=> MaintenanceTasks::CsvCollectionBuilder).

I couldn't write a test to cause the exception though (using Rails.application.reloader.reload! or even including the Rails::ConsoleMethods and calling reload! wasn't working from a rake task for some reason).

I'm conflicted because this particular means of testing autoloading has been failing forever basically (since v1.1.1), but in general autoloading from the UI works, and you can run your task. I mean it's pretty bad that we broke reload!, but you could run your tasks with the classic autoloader, now the UI is entirely broken.
On the other hand, migrating to Zeitwerk doesn't seem too hard on users, and we can avoid having to support it…

I don't know I just feel bad dropping support abruptly. I guess we'll see if we get some bug reports whenever this is released.

[gmcgibbon]

I'm happy to revert and log a deprecation instead in the next release, if we think it is necessary.

[adrianna-chang-shopify]

On the other hand, migrating to Zeitwerk doesn't seem too hard on users, and we can avoid having to support it…

My feeling was that we'd been supporting classic autoloading in a half-broken manner up until recently with no complaints, and support for non-Zeitwerk apps is going to be dropped soon with Rails 7, so apps should be focused on migrating anyways.

I agree that dropping it with no notice is probably bad OSS etiquette though 😅

So maybe we should revert, qualify the constant to fix reloading, and soft deprecate for now?

[gmcgibbon]

Ok, I'll revert and warn instead.