Fix CLI init by defining default load path value

[mikelkew]

What are you trying to accomplish?

Resolves #76.

ConfigurationFile#initialize is expecting an Array value for load_paths, yet when called via the packwerk init CLI command, it was being passed as nil and thus throwing a Sorbet type violation.

What approach did you choose and why?

I've just provided an empty array as a fallback if no load_paths value is defined, which is the case when running the config generator. Seemed like the most logical solution and matches the approach used for custom_associations.

What should reviewers focus on?

I assume an empty array is the desired fallback here, although I guess ApplicationLoadPaths.extract_relevant_paths could also be an option.

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)

Checklist

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

[mikelkew]

Note, CLA has been signed. Tried reopening the PR to trigger a re-check of CLA status, but no luck..

[doodzik]

This change looks good to me 👍 Thank you for fixing the issue.
I would like to get @wildmaples or @rochlefebvre to have a look at this PR before we go ahead and merge it because I'm unsure about the default value.

[rochlefebvre]

The Configuration class recently stopped inferring a set of load paths unless packwerk.yml file specified one:
https://github.com/Shopify/packwerk/pull/68/files#r524332054 . It now expects the calling code to provide a list of paths. Evidently, not all callers provide one.

For non-Rails applications, your change reinstates the previous behaviour of setting load_paths = [] by default:

As for Rails applications, I think only the ApplicationValidator class will notice that Configuration#load_paths is inconsistent with Rails' own load paths:

packwerk/lib/packwerk/application_validator.rb

Lines 50 to 63 in 88526cc

	def check_autoload_path_cache
	expected = @application_load_paths
	actual = @configuration.load_paths
	if expected.sort == actual.sort
	Result.new(true)
	else
	Result.new(
	false,
	"Load path cache in #{@config_file_path} incorrect!\n"\
	"Paths missing from file:\n#{format_yaml_strings(expected - actual)}\n"\
	"Extraneous load paths in file:\n#{format_yaml_strings(actual - expected)}"
	)
	end
	end

I think your change is a step in the right direction, @mikelkew . Do you have anything to add, @wildmaples ?

[wildmaples]

I don't think the proposed solution in this PR is correct. By adding this change, we are not generating any load_paths for packwerk init. Instead it will just return an empty list. The correct behaviour is to get a list of load paths through Rails, and populate the configuration file.

We'll have to change the load path argument on this line:

packwerk/lib/packwerk/cli.rb

Line 92 in 1489086

load_paths: @configuration.load_paths,

to use the new ApplicationLoadPaths#extract_relevant_paths method.

[mikelkew]

Thanks @wildmaples, I had a suspicion that might be the desired behaviour. I've just pushed b9844f7 to address it (keeping the PR as a single commit as preferred).

I've intentionally kept the empty array as a fallback for load_paths when loading a configuration file though. ApplicationValidator#check_autoload_path_cache expects this to be an array, so this covers us if an existing config file doesn't have it defined.

[rochlefebvre]

Calling ApplicationLoadPaths.extract_relevant_paths in a non-Rails app will assert, So your solution is halfway there:

packwerk/lib/packwerk/application_load_paths.rb

Lines 12 to 23 in 88526cc

	def extract_relevant_paths
	assert_application_booted
	all_paths = extract_application_autoload_paths
	relevant_paths = filter_relevant_paths(all_paths)
	assert_load_paths_present(relevant_paths)
	relative_path_strings(relevant_paths)
	end
	sig { void }
	def assert_application_booted
	raise "The application needs to be booted to extract load paths" unless defined?(::Rails)
	end

Somewhere in the generate configs logic, we're missing this pseudocode

generated_load_paths = rails_app? ? ApplicationLoadPaths.extract_relevant_paths : []

Solution 1

The ApplicationLoadPaths class is already aware of the application's potential for not being a Rails app, and raises when that's the case. Having extract_relevant_paths return early with [] instead of asserting could work, and that's all we'd need to fix your PR. ApplicationLoadPaths remains the source of truth for load path determination, even if it's to say ¯_(ツ)_/¯.

Solution 2

The CLI should not be in the business of implementing the decision modeled in the pseudocode. But there is precedent for calling a generator with a :for_rails_app hint:

packwerk/lib/packwerk/cli.rb

Lines 80 to 87 in 88526cc

	def init
	@out.puts("📦 Initializing Packwerk...")
	application_validation = Packwerk::Generators::ApplicationValidation.generate(
	for_rails_app: rails_app?,
	root: @configuration.root_path,
	out: @out
	)

Say we passed the same hint into Packwerk::Generators::ConfigurationFile.generate and pushed the default load paths decision into that generator:

# lib/packwerk/cli.rb
Packwerk::Generators::ConfigurationFile.generate(
  for_rails_app: rails_app?,
  root: @configuration.root_path,
  out: @out
)

# lib/packwerk/generators/configuration_file.rb
class << self
  def generate(for_rails_app:, root:, out:)
    new(for_rails_app: for_rails_app, root: root, out: out).generate
  end
end

def initialize(for_rails_app:, root:, out: $stdout)
  @for_rails_app = for_rails_app
  @root = root
  @out = out

  set_template_variables
end

def load_paths
  @load_paths ||= @for_rails_app ? Packwerk::ApplicationLoadPaths.extract_relevant_paths : []
end

def set_template_variables
  @load_paths_formatted = if load_paths.empty?
    "# load_paths:\n# - 'app/models'\n"
  else
    load_paths.map { |path| "- #{path}\n" }.join
  end

  @load_paths_comment = unless load_paths.empty?
    "# These load paths were auto generated by Packwerk.\nload_paths:\n"
  end
end

I have a preference for the first solution.

[mikelkew]

Thanks for catching that @rochlefebvre. I've gone with your first suggestion, both for simplicity, and because it matches the behaviour for non-rails apps that was in place before #68 was merged.

[wildmaples]

Is there a way to return [] outside this class instead of in this early return?

defined?(::Rails) is really misleading because it (almost) always this will be true, as Packwerk itself has Rails and will fall back to that. This is a known "bug". I am not sure why we even still have this check. @exterm @tomstuart

In the case where defined?(::Rails) works the way we intend it too though:

In the context of ApplicationLoadPaths, we would still want to raise if Rails is not defined because the point of the class is to get the Rails load paths. Returning an empty array would be misleading users to the actual functionality / API of this class.

TL;DR: We should not return empty array here but instead outside of this class, as it would be misleading to the functionality of this class. We should also keep the raise instead of failing silently because this class won't get its intended functionality if we don't have Rails defined.

edit: ignore my comments about Rails not being defined, that check works 😄

[wildmaples]

As per my latest comment, we can just leave this class as is and come back to add the Rails loading logic in another PR. When that is added, we won't have to raise when Rails is not defined, and just return an empty array as you proposed here.

We could make the change now and return an empty array, but it would be hiding the fact that Rails should be defined/loaded but is not.

Unless someone has strong opinions, I say we keep the raise until we can make that change.

[mikelkew]

Ok, thanks @wildmaples, implemented in fe1697e

If we want to avoid raising for non-rails apps in the meantime, then I could implement the second suggestion from @rochlefebvre instead. But if the PR for the Rails logic is going to be worked on sooner rather than later, perhaps the raise isn't too much of a concern for now.

[wildmaples]

I had a chat with @rafaelfranca

There are two bugs here: the first is solved by this line of change.

The second issue is that bundle exec packwerk / packwerk and bin/packwerk packwerk produces different behaviour in Packwerk. bin/packwerk loads Rails files, while bundle exec packwerk does not.

We previously had bin/packwerk because we wanted to keep the Rails part of the code separate from Packwerk as it may be used in a Ruby project. Now that Packwerk presumably only works with Rails (sorry, team Ruby), the solution is to load the Rails app within the ApplicationLoadPaths class.

I will add an issue for this sometime tomorrow. In the meantime, I think the appropriate change is the line I linked above.

[rochlefebvre]

This PR addresses the Sorbet violation when initializing a ConfigurationFile, at least within a Rails app. It breaks packwerk init and packwerk generate_configs for non-Rails applications, now that Packwerk::Generators::ConfigurationFile.generate_configs relies on ApplicationLoadPaths. We intend to track and fix that in a separate issue/PR.

Having Configuration return [] unless provided some load_paths prevents a NoMethodError in ApplicationValidator#check_autoload_path_cache, even if the check itself will fail, as designed.

[apeacock1991]

Just stumbled upon this as I've just tried to install on a brand new Rails app and got the same error, is this going to be merged soon / tagged to fix the issue? As currently it's impossible (I think) to install Packwerk on any Rails app 😬

[wildmaples]

👍 Thanks, @mikelkew