Switch application load path finding to zeitwerk

[shageman]

What are you trying to accomplish?

Packwerk currently uses Rails.autoloaders to determine the load paths it should include in its analysis.

For unbuilt gems that are added to the application via the path: option in the Gemfile, this means that they have be turned into an engine for their paths to be picked up. One can either fully turn the gem into an engine or do so opportunistically when Rails is defined like so:

if defined?(Rails)
  require 'testgem/engine'
else
  require 'zeitwerk'
  loader = Zeitwerk::Loader.new
  loader.tag = File.basename(__FILE__, '.rb')
  loader.inflector = Zeitwerk::GemInflector.new(__FILE__)
  app_paths = Dir.glob(File.expand_path(File.join(__dir__, '../app', '/*')))
  app_paths.each { |k| loader.push_dir(k) }
  loader.setup
end

Here, for tests, zeitwerk is used to ensure that loading is the same (similar?) between tests and prod.

That if statement is quite gross ... and unnecessary!

This PR switches path finding from Rails.autoloaders to Zeitwerk::Registry.loaders, with which the if statement is no longer necessary and the gem doesn't need to be turned into an engine.

What approach did you choose and why?

This PR makes zeitwerk a runtime dependency of packwerk. By switching from Rails.autoloaders to Zeitwerk::Registry.loaders any gem that uses zeitwerk for its file loading will be included in hackwork's analysis.

What should reviewers focus on?

I have checked this against a large codebase at gusto. Performance and results are the same. I think it makes sense for others to do the same.

Type of Change

• Bugfix
• New feature
• Non-breaking change (a change that doesn't alter functionality - i.e., code refactor, configs, etc.)

Additional Release Notes

• Breaking change (fix or feature that would cause existing functionality to change)

Unbuilt gems in a codebase that use zeitwerk will not show up in packwerk violations before this and will show up in violations afterwards.

Checklist

• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• It is safe to rollback this change.

[shageman]

I might not be able to solve the CLA problem... I am already signed up and am not sure what the problem is.

[fxn]

Hey, this patch is using private API:

I mark private API very carefully, and a patch release can change any of it.

I am not familiar with packwerk itself, would need some help understanding the problem (beyond what the description says). What are those paths for? Can packwerk define an API to register components if this is saying it needs one?

[shageman]

Thank you for the review, Xavier!

I feel like I found the line in a blog post (I will try to find that) and didn't look at the source code, so wasn't aware of the privateness.

Packwerk operates on a set of directories to find constants defined in the app for which we can then define package boundaries and make dependencies explicit. Any normally autoloader Rails folder works, any that devs add to the config works, even engines in the app repo that are added to the Gemfile via path: work. Gems in the app repo that are not engines do not work.

Packwerk doesn't have a config for these folders, it finds them. In most practical scenarios the folders change too frequently for it to not be a nuisance to manage manually.

Is there any public way to ask zeitwerk for all the folders it manages?

[rafaelfranca]

Thank you for the pull request. Packwerk being a library for Rails applications has as requirement that libraries that needs to integrate with the Rails application have a least a Rails::Railtie defined.

As one of the main reason why someone would use packwerk is to enable modularity, Rails::Engines are the perfect way to do that since they can define their own configuration and initializers, isolating them from the Rails application.

I personally don't see any downside of requiring gems that want to be understood by packwerk to define a Rails engine, so I prefer to not change this implementation.

[mclark]

Hmmm this isn't going to work for us. Having the default namespace specified for each root dir is a feature GitHub depends on quite a bit.

I think using Zeitwerk::Registry will work (and if so, it sounds much nicer, pending the feedback above about it being considered private) but I gotta insist on retaining the Hash of paths to modules. Hope that's ok.

[fxn]

I'm willing to help if new use cases arise. For example, if a read-only collection of root directories with their associated namespaces would be a good solution for what this is trying to solve, I'd be open to implement it.

However, I don't know if this is a good solution to the root problem. If a gem is not an engine, and does not use Zeitwerk either, what?

Take also into account that Zeitwerk is used by gems. How are you going to distinguish the loaders that matter among them?

[shageman]

@mclark it feels like we should have used packwerk to define which parts of packwerk are public API and which are not ;) jkjk

Do you mind elaborating as to what you do with the default namespace?

Thanks for offering to support this, Xavier! To answer your questions:

If a gem is not an engine, and does not use Zeitwerk either, what?

Such gems would currently not be included. And I think that is fine because...

Take also into account that Zeitwerk is used by gems. How are you going to distinguish the loaders that matter among them?

... currently packwerk only operates on constants defined with the Rails application. I.e., packwerk is focussed on helping engineers understand and clean up the dependency structures within their own applications, so there is already a boundary built in. #216 is looking to change this and add more options, but the fundamental fact that packwerk has controls for this won't change.

How is any of this useful? One example of an app that uses gems and engines as part of the codebase is https://github.com/instructure/canvas-lms/tree/master/gems, which defines a bunch of gems inline and uses the directly from the Gemfile (https://github.com/instructure/canvas-lms/blob/master/Gemfile.d/app.rb#L160)

[mclark]

@shageman sorry I missed your comment somehow!

Do you mind elaborating as to what you do with the default namespace?

Some legacy directories in GitHub do not fully conform to the patterns established by Zeitwerk and have an implicit "root" namespace. When we migrated to Zeitwerk we leveraged the ability to set a default namespace for each root directory in order to override the default Object root in these cases.

For Packwerk to resolve constants properly, it too needs to know what the default root namespaces are otherwise the expected and actual namespaces for each file will not match.

This isn't an ideal situation, but for the foreseeable future we will be stuck with this pattern unfortunately. Thanks for taking the time to understand the problem.

[fxn]

Let me say that I believe root namespaces are as good practice as Object. If I find an API that I like and is backwards compatible, I'd like to provide configuration for them in Rails somehow. Nowadays, you can do this reaching for the autoloaders by hand, but I believe this should be better supported.

I have been consulting for many years, and it is rare the project that does not have app in the autoload paths. That is a weird configuration, and one that is forced because teams just one app/api to be API, for example. The fact that app/models is not a namespace does not mean people do not want app/components to be one, because we'll all agree app/components/components is screaming the current settings are not sufficient.

[rafaelfranca]

Suggested change

	Packwerk is a Ruby gem used to enforce boundaries and modularize Ruby applications (including Rails apps!).
	Packwerk is a Ruby gem used to enforce boundaries and modularize Rails applications.

We have no interest of entering the business of modularize arbitrary Ruby applications. I'm ok to allow load path discovery using Zeitwerk given that makes sense in the context of a Rails application, but if people start to ask for "feature X to support Hanami apps" or "Feature Y to support Sinatra apps" the answer at the moment will be an automatic "sorry, you are on your own". So, it is better to not claim in the README that we Packwerk is something it isn't.

[shageman]

Makes sense! I know a lot happened today and this PR is no longer relevant for the moment, but I will make sure to have this fixed when we have an acceptable solution in place.

[fxn]

We had a followup Zoom call with @shageman and @alexevanczuk to discuss a bit ideas around this.

Possible directions without Zeitwerk

AFAICT, packwerk does not really depend on Zeitwerk. Please correct me if I am mistaken, but I believe it basically works with root directories and assume the naming conventions of projects managed by Zeitwerk (except for collapsing or custom inflectors).

If that was correct, I believe a clean way to open packwerk to work with any gem would be to provide API to push root directories. Then, the gem would be responsible for registering its root paths, even if it is only lib.

If the gem is managed by Zeitwerk, fine. If it is not, fine too as long as it follows the conventions.

If you need support for namespaces associated to root directories, that is as small tweak, and Object could be a default.

Simplification assuming Zeitwerk

Alternatively, if you want to support only projects managed by Zeitwerk, then the gem could simply register its autoloaders (there can be more than one), and packwerk could introspect and even use their inflectors.

Not implying this is something wanted

Not saying packwerk should do this, only giving feedback :).