Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Switch application load path finding to zeitwerk #218

Closed
wants to merge 5 commits into from
Closed

Switch application load path finding to zeitwerk #218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d99f33b
Switch application load path finding to zeitwerk
shageman Aug 11, 2022
9c2b13e
Add gitkeep for new gem folder in skeleton
shageman Aug 11, 2022
c8afec5
Fix rubocop issues
shageman Aug 11, 2022
a0ffece
“Fixed” the readme ;)
shageman Aug 11, 2022
db2bfd0
Use zeitwerk’s public API for finding app load paths
shageman Aug 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ PATH
parallel
parser
sorbet-runtime (>= 0.5.9914)
zeitwerk

GEM
remote: https://rubygems.org/
Expand Down Expand Up @@ -258,7 +259,6 @@ DEPENDENCIES
sorbet-runtime
spring
tapioca
zeitwerk

BUNDLED WITH
2.3.5
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ We are keeping `packwerk` compatible with current versions of Ruby and Rails, bu

-- _Sandi Metz, Practical Object-Oriented Design in Ruby_

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

@rafaelfranca rafaelfranca Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Packwerk can be used to:
* Combine groups of files into packages
Expand Down
20 changes: 9 additions & 11 deletions lib/packwerk/application_load_paths.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ module ApplicationLoadPaths
class << self
extend T::Sig

sig { params(root: String, environment: String).returns(T::Hash[String, Module]) }
sig { params(root: String, environment: String).returns(T::Array[String]) }
def extract_relevant_paths(root, environment)
require_application(root, environment)
all_paths = extract_application_autoload_paths
Expand All @@ -18,30 +18,28 @@ def extract_relevant_paths(root, environment)
relative_path_strings(relevant_paths)
end

sig { returns(T::Hash[String, Module]) }
sig { returns(T::Array[String]) }
def extract_application_autoload_paths
Rails.autoloaders.inject({}) do |h, loader|
h.merge(loader.root_dirs)
end
Zeitwerk::Loader.all_dirs
Copy link
Contributor

@mclark mclark Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@fxn fxn Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end

sig do
params(all_paths: T::Hash[String, Module], bundle_path: Pathname, rails_root: Pathname)
.returns(T::Hash[Pathname, Module])
params(all_paths: T::Array[String], bundle_path: Pathname, rails_root: Pathname)
.returns(T::Array[Pathname])
end
def filter_relevant_paths(all_paths, bundle_path: Bundler.bundle_path, rails_root: Rails.root)
bundle_path_match = bundle_path.join("**")
rails_root_match = rails_root.join("**")

all_paths
.transform_keys { |path| Pathname.new(path).expand_path }
.map { |path| Pathname.new(path).expand_path }
.select { |path| path.fnmatch(rails_root_match.to_s) } # path needs to be in application directory
.reject { |path| path.fnmatch(bundle_path_match.to_s) } # reject paths from vendored gems
end

sig { params(load_paths: T::Hash[Pathname, Module], rails_root: Pathname).returns(T::Hash[String, Module]) }
sig { params(load_paths: T::Array[Pathname], rails_root: Pathname).returns(T::Array[String]) }
def relative_path_strings(load_paths, rails_root: Rails.root)
load_paths.transform_keys { |path| Pathname.new(path).relative_path_from(rails_root).to_s }
load_paths.map { |path| Pathname.new(path).relative_path_from(rails_root).to_s }
end

private
Expand All @@ -59,7 +57,7 @@ def require_application(root, environment)
end
end

sig { params(paths: T::Hash[T.untyped, Module]).void }
sig { params(paths: T::Array[Pathname]).void }
def assert_load_paths_present(paths)
if paths.empty?
raise <<~EOS
Expand Down
2 changes: 1 addition & 1 deletion lib/packwerk/run_context.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def from_configuration(configuration)
sig do
params(
root_path: String,
load_paths: T::Hash[String, Module],
load_paths: T::Array[String],
inflector: T.class_of(ActiveSupport::Inflector),
cache_directory: Pathname,
config_path: T.nilable(String),
Expand Down
2 changes: 1 addition & 1 deletion packwerk.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,13 @@ Gem::Specification.new do |spec|
spec.add_dependency("constant_resolver", ">=0.2.0")
spec.add_dependency("parallel")
spec.add_dependency("sorbet-runtime", ">=0.5.9914")
spec.add_dependency("zeitwerk")

spec.add_development_dependency("m")
spec.add_development_dependency("rake")
spec.add_development_dependency("sorbet")
# https://github.com/ruby/psych/pull/487
spec.add_development_dependency("psych", "~> 3")
spec.add_development_dependency("zeitwerk")

# For Ruby parsing
spec.add_dependency("ast")
Expand Down
Empty file added test/fixtures/skeleton/gems/gem_a/.gitkeep
View file
Open in desktop
Empty file.
40 changes: 33 additions & 7 deletions test/unit/application_load_paths_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,44 +11,70 @@ class ApplicationLoadPathsTest < Minitest::Test
relative_path = "app/models"
absolute_path = rails_root.join(relative_path)
relative_path_strings = ApplicationLoadPaths.relative_path_strings(
{ absolute_path => Object },
[absolute_path],
rails_root: rails_root
)

assert_equal({ relative_path => Object }, relative_path_strings)
assert_equal([relative_path], relative_path_strings)
end

test ".filter_relevant_paths excludes paths outside of the application root" do
valid_paths = ["/application/app/models"]
paths = valid_paths + ["/users/tobi/.gems/something/app/models", "/application/../something/"]
paths = paths.each_with_object({}) { |p, h| h[p.to_s] = Object }
# paths = paths.each_with_object([]) { |p, h| h << p.to_s }
filtered_paths = ApplicationLoadPaths.filter_relevant_paths(
paths,
bundle_path: Pathname.new("/application/vendor/"),
rails_root: Pathname.new("/application/")
)

assert_equal({ "/application/app/models" => Object }, filtered_paths.transform_keys(&:to_s))
assert_equal(["/application/app/models"], filtered_paths.map(&:to_s))
end

test ".filter_relevant_paths excludes paths from vendored gems" do
valid_paths = ["/application/app/models"]
paths = valid_paths + ["/application/vendor/something/app/models"]
paths = paths.each_with_object({}) { |p, h| h[p.to_s] = Object }
# paths = paths.each_with_object({}) { |p, h| h[p.to_s] = Object }
filtered_paths = ApplicationLoadPaths.filter_relevant_paths(
paths,
bundle_path: Pathname.new("/application/vendor/"),
rails_root: Pathname.new("/application/")
)

assert_equal({ "/application/app/models" => Object }, filtered_paths.transform_keys(&:to_s))
assert_equal(["/application/app/models"], filtered_paths.map(&:to_s))
end

test ".extract_relevant_paths calls out to filter the paths" do
ApplicationLoadPaths.expects(:filter_relevant_paths).once.returns(Pathname.new("/fake_path").to_s => Object)
ApplicationLoadPaths.expects(:filter_relevant_paths).once.returns([Pathname.new("/fake_path").to_s])
ApplicationLoadPaths.expects(:require_application).with("/application", "test").once.returns(true)

ApplicationLoadPaths.extract_relevant_paths("/application", "test")
end

test ".extract_relevant_paths uses Zeitwerk loaders" do
result = ApplicationLoadPaths.extract_relevant_paths("./test/fixtures/skeleton", "test")
assert_equal([
"components/sales/app/models",
"components/platform/app/models",
"components/timeline/app/models",
"components/timeline/app/models/concerns",
"vendor/cache/gems/example/models",
],
result)

loader = Zeitwerk::Loader.new
loader.push_dir("./test/fixtures/skeleton/gems/gem_a")

result = ApplicationLoadPaths.extract_relevant_paths("./test/fixtures/skeleton", "test")
assert_equal([
"components/sales/app/models",
"components/platform/app/models",
"components/timeline/app/models",
"components/timeline/app/models/concerns",
"vendor/cache/gems/example/models",
"gems/gem_a",
],
result)
end
end
end
6 changes: 3 additions & 3 deletions test/unit/cli_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class CliTest < Minitest::Test
offense = Offense.new(file: file_path, message: violation_message)

configuration = Configuration.new({ "parallel" => false })
configuration.stubs(load_paths: {})
configuration.stubs(load_paths: [])
RunContext.any_instance.stubs(:process_file).at_least_once.returns([offense])

string_io = StringIO.new
Expand All @@ -49,7 +49,7 @@ class CliTest < Minitest::Test
offense = Offense.new(file: file_path, message: violation_message)

configuration = Configuration.new({ "parallel" => false })
configuration.stubs(load_paths: {})
configuration.stubs(load_paths: [])

RunContext.any_instance.stubs(:process_file)
.at_least(2)
Expand Down Expand Up @@ -137,7 +137,7 @@ def show_stale_violations(_offense_collection, _fileset)
offense = Offense.new(file: file_path, message: violation_message)

configuration = Configuration.new
configuration.stubs(load_paths: {})
configuration.stubs(load_paths: [])
RunContext.any_instance.stubs(:process_file)
.returns([offense])

Expand Down