Upgrading zeitwerk to 2.6.0 causes a warning related to good_job

[jgrau]

Hi @bensheldon

Today I upgraded zeitwerk to v. 2.6.0. Running my testsuite now produces

WARNING: Zeitwerk defines the constant ActiveJob after the directory

    /Users/jgrau/.rbenv/versions/3.0.2/lib/ruby/gems/3.0.0/gems/good_job-2.15.1/lib/active_job

To prevent that, please configure the loader to ignore it:

    loader.ignore("#{__dir__}/active_job")

Otherwise, there is a flag to silence this warning:

    Zeitwerk::Loader.for_gem(warn_on_extra_files: false)

The zeitwerk changelog describes the warning. It probably makes more sense to you than to me ;)

[sedubois]

I don't understand the warning, as it seems to contradict the Zeitwerk documentation on third-party namespaces, which GoodJob already seems to follow.

[nickcampbell18]

I came here to say the same thing - I think the documentation was not synchronised with this change.

I had a quick play locally, using the loader.ignore(...) method won't work because it excludes the directory altogether. Disabling the warning is probably the better approach, however this flag was only added in v2.6.0 so good_job would need to require >= 2.6.0 in the gemspec (and I don't know if this is a good thing for compatibility with other systems also dependent on zeitwerk).

[sedubois]

• Rails 7's railties depends on zeitwerk ~> 2.5
• Rails 6.1's activesupport depends on zeitwerk ~> 2.3
• Rails 6.0's activesupport depends on zeitwerk ~> 2.2, >= 2.2.2
• Earlier Rails don't use zeitwerk

[bensheldon]

Thanks everyone for flagging and discussing 🙌 This is really helpful for me. I see two different directions to go:

• Do a version check on Zeitwerk and initialize it with the warn_on_extra_files: false flag. That seems brittle, but such is gem development where I can't tightly control dependencies.
• Ignore the lib/active_job directory and manually require the only file that lives in that namespace, which is ActiveJob::QueueAdapters::GoodJobAdapter. I'm sympathetic to the autoloader complaint; I would rather not open up those namespaces prematurely just to drop my constant if I can avoid it either.

I am leaning towards the 2nd option. I will see if I can require the ActiveJob::QueueAdapters::GoodJobAdapter in an initializer after ActiveJob initializes.

[fxn]

Let me explain.

With the pass of time I've seen a few gems with extra directories under lib that should not be managed by Zeitwerk. This may have unwanted side-effects like defining namespaces you do not intend to (and should not) define.

Typical example is a gem that provides Rails generators:

lib/rails/generators/my_generator.rb
lib/my_gem
lib/my_gem.rb

1. The file lib/rails/generators/my_generator.rb does NOT define Rails::Generators::MyGenerator, just MyGenerator.
2. Rails is going to issue require call searching for it, this is not meant to be autoloadable (docs).
3. Zeitwerk would define Rails::Generators as a side-effect of the overlook, but the gem should not define such thing.

Bottom line, that lib/rails should be ignored. So, in 2.6.0 we start warning about extra stuff under lib.

In this particular case, lib/active_job is not an overlook, it is a real directory that has to be managed by the loader. The correct way to address this is to say: "I know what I am doing, the warning does not need to be issued":

Zeitwerk::Loader.for_gem(warn_on_extra_files: false)

The documentation has been updated in fxn/zeitwerk@9daa7b8.

[fxn]

@bensheldon Just read your reply. Your 2nd option is totally cool too.

[bensheldon]

@fxn thank you for hopping in this issue ❤️ Given that the Zeitwerk Readme uses the example of an ActiveJob adapter, I think I will instead follow your instructions 😄

https://github.com/fxn/zeitwerk#reopening-third-party-namespaces

[bensheldon]

@fxn one more thought. I'd like to avoid forcing a Zeitwerk dependency upgrade. Does this seem like a reasonable version check?

zeitwerk_options = Gem::Version.new(Zeitwerk::VERSION) >= Gem::Version.new("2.6.0") ? { warn_on_extra_files: false } : {}
Zeitwerk::Loader.for_gem(**zeitwerk_options).tap do |loader|

[bensheldon]

This also seems to work as a workaround. I worry that implying to Zeitwerk that "I know what I'm doing" with warn_on_extra_files: true is hubris 😓

Zeitwerk::Loader.for_gem.tap do |loader|
  loader.inflector.inflect({
                             "cli" => "CLI",
                           })
  loader.ignore("#{__dir__}/generators")
  loader.ignore("#{__dir__}/active_job")
  loader.push_dir("#{__dir__}/active_job/queue_adapters", namespace: ActiveJob::QueueAdapters)
  loader.setup
end

[fxn]

@bensheldon I like this one even as a replacement for the documentation, because it forces the user to require the namespace beforehand or it won't work.

EDIT: Actually no, when I read the snippet I didn't realize it was ignoring active_job. This configuration should not even work the way it does now, because you have active code within an ignored directory.

[fxn]

@bensheldon what do you think about

Zeitwerk::Loader.for_gem.tap do |loader|
  loader.inflector.inflect({
                             "cli" => "CLI",
                           })
  loader.ignore("#{__dir__}/generators")
  loader.push_dir("#{__dir__}/good_job/adapter", namespace: ActiveJob::QueueAdapters)
  loader.setup
end

?

[bensheldon]

@fxn hmmm. I don't really like mixing auto and manually nested constants in the same good_job directory. I worry I'll forget the reason why and violate my own principle of least surprise at some point in the future.

I think I prefer doing both ignore and push_dir-with-namepace because they are clearest to me of what's happening and they keep the constants nested consistently without exception.

[fxn]

On the other hand it feels weird to me to have active code below an ignored directory.

[fxn]

Let me also say that the flag does not have to feel like hubris at all, eh? That is a pretty standard thing. You have a warning in tools for something that commonly is worth mentioning, but you may be in a situation where the warning is not warranted, and the tool gives you a way to disable.

[bensheldon]

@fxn thank you for that 🙏 I deployed the version with the ignore+push_dir. I guess in my mental model of Zeitwerk I'm opting out of the default code-loading algorithm, and then declaring a special-purpose one.

[fxn]

@bensheldon problem is, this configuration is inconsistent and I'll do something about it. Either raise if a root directory is a descendant of an ignored one, or ignore that root directory on traversal. In either case, your patch would not work.

So, I see three options:

1. Use the flag. This is exactly the use case for it.
2. Move the adapter to the gem directory and configure the namespace for it.
3. Ignore and issue a require call for the adapter, the user loaded the gem, so the constant is expected anyway.

From them, (1) is the most simple and the recommended one after sleeping on it. But the other two are good too and aligned with the autoloading interface.

[fxn]

There is a 4th option, which is to use a regular Zeitwerk::Loader configured by yourself, since for_gem is only a convenience shortcut as documented here. Regular loaders do not issue warnings.

But, really, the flag is the idiomatic one.

[bensheldon]

@fxn ok sounds like I should use the flag 😄 Thank you again for the help and discussion here 🙏

Just in case it got lost, my reluctance for the flag is less around turning it off (though I do like helpful warnings), so much as it is about the need for a hard version check to turn it off. e.g.

zeitwerk_options = Gem::Version.new(Zeitwerk::VERSION) >= Gem::Version.new("2.6.0") ? { warn_on_extra_files: false } : {}
Zeitwerk::Loader.for_gem(**zeitwerk_options).tap do |loader|
#...

As an argument, unless I'm mistaken (is there a standard way to introspect on args?), it's not possible to do feature checking and thus requires the messier (imo) version checking. Alternatively for example, If it was, for example, a class configuration it would make me less worried about introducing a future problem:

Zeitwerk::Loader.warn_on_extra_files = false if Zeitwerk::Loader.respond_to?(:warn_on_extra_files=)

[fxn]

Hey! Maybe parameters would help?

Zeitwerk::Loader.method(:for_gem).parameters

returns [] before 2.6.0, and [[:key, :warn_on_extra_files]] now.

[fxn]

What do you think about an allow list instead? Something like

loader.do_not_warn_about("#{__dir__}/active_job")

possibly with a better name? Would that be a better interface for you?

[bensheldon]

@fxn I think that's an improvement. I think it still leaves me with the future thought "Why?" and I'd probably end up writing it like this as a note to my future self:

loader.do_not_warn_about("#{__dir__}/active_job") # already required constants above

Would it be possible to make the flag assertive of the prevention? The following is some sloppy wording, but hopefully the suggestion makes sense:

e.g. loader.already_required_external_constants_within("#{__dir__}/active_job}")

That would create some symmetry between what's happening outside of the loader configuration block, and within. (unless I'm utterly misunderstanding the source of the warning).

require 'active_job'
require 'active_job/queue_adapters'

Zeitwerk::Loader.for_gem.tap do |loader|
  loader.already_required_external_constants)_within("#{__dir__}/active_job")
  loader.setup
end

Or to try to move the configuration even closer together, would it be possible to do the require itself within the configuration block? e.g.something like

loader.prerequire_constants("#{__dir__}/active_job") do
  require 'active_job'
  require 'active_job/queue_adapters'
end

[fxn]

It's a tough one, that is why I went with the simplest solution.

Most of the time anything extra should be ignored (rails, core_extensions, generators, tasks, etc.), I've seen these and it is what motivated this feature. Use cases for gems with legit extra stuff are less common. Even reopening a namespace from a separate gem is exceptional, normally source trees are disjoint.

However, I cannot tell from the existence of the constant if the directory is legit to not warn (Rails exists), and cannot give it a too specific API because maybe there are more use cases for legit directories than reopening stuff.

I'll think about it, thanks for the discussion 😀.