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.

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

Require Zeitwerk 2.6.1 #258

Merged
merged 1 commit into from
Nov 7, 2022
Merged

Require Zeitwerk 2.6.1 #258

merged 1 commit into from
Nov 7, 2022

Conversation

gmcgibbon
Copy link
Member

@gmcgibbon gmcgibbon commented Nov 7, 2022

What are you trying to accomplish?

Followup on #251.

Require Zeitwerk 2.6.1 so that we can always use the public introspection API to get application autoload paths.

What approach did you choose and why?

The loader approach previously used was good, but if we can just require a later version of Zeitwerk, we don't need to write branching logic for each version.

What should reviewers focus on?

Is there any reason not to impose a minimum version of Zeitwerk for the gem?

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)

Include any notes here to include in the release description. For example, if you selected "breaking change" above, leave notes on how users can transition to this version.

If no additional notes are necessary, delete this section or leave it unchanged.

Checklist

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

@gmcgibbon gmcgibbon requested a review from a team as a code owner November 7, 2022 21:31
Copy link
Contributor

@alexevanczuk alexevanczuk left a comment

Choose a reason for hiding this comment

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

This seems fine but would we call this a breaking change? We're removing support for <2.6.4, so I thought this could be released in the major version.

@rafaelfranca
Copy link
Member

Not a breaking change since upgrading to Zeitwerk 2.6.4 should not break any app.

@gmcgibbon gmcgibbon force-pushed the require_zeitwerk_2_6_4 branch from 312b411 to efde788 Compare November 7, 2022 23:02
@@ -20,8 +20,8 @@ def extract_relevant_paths(root, environment)

sig { returns(T::Hash[String, Module]) }
def extract_application_autoload_paths
Loader.autoloaders.inject({}) do |h, loader|
h.merge(loader.dirs(namespaces: true))
Rails.autoloaders.inject({}) do |h, autoloader|
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Rails.autoloaders.inject({}) do |h, autoloader|
Rails.autoloaders.inject({}) do |h, loader|

Loader.autoloaders.inject({}) do |h, loader|
h.merge(loader.dirs(namespaces: true))
Rails.autoloaders.inject({}) do |h, autoloader|
h.merge(autoloader.dirs(namespaces: true))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
h.merge(autoloader.dirs(namespaces: true))
h.merge(loader.dirs(namespaces: true))

Copy link
Member

Choose a reason for hiding this comment

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

Loader is more correct here since those loaders can autoload or eagerload.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder why Rails uses autoloaders naming then. 🤔 Will update!

@gmcgibbon
Copy link
Member Author

gmcgibbon commented Nov 7, 2022

This seems fine but would we call this a breaking change? We're removing support for <2.6.4, so I thought this could be released in the major version.

I wasn't 100% sure if adding version constraints is a breaking change, that's why I suggested the conditional logic in the original PR. However, I think Rafael is correct, bundler should be able to decide if an app can meet the version constraint or not.

Rails' constraint is "zeitwerk", "~> 2.5" as of 7.0, and that's on railties which we don't depend on in the gemspec. So, we should be specifying a zeitwerk dependency regardless if we ever want to support non-rails usage of Packwerk (and since we depend so heavily on it). Additionally, we reference Rails.root but don't depend on railties, so there's a few things still potentially wrong here. For now, I think adding the constraint for Zeitwerk makes the most sense to fix the bug we encountered.

Require Zeitwerk 2.6.1 so that we can always use the public
introspection API to get application autoload paths.
@gmcgibbon gmcgibbon force-pushed the require_zeitwerk_2_6_4 branch from efde788 to f5d5af4 Compare November 7, 2022 23:23
@gmcgibbon gmcgibbon changed the title Require Zeitwerk 2.6.4 Require Zeitwerk 2.6.1 Nov 7, 2022
@gmcgibbon gmcgibbon merged commit ae762f0 into main Nov 7, 2022
@gmcgibbon gmcgibbon deleted the require_zeitwerk_2_6_4 branch November 7, 2022 23:30
gmcgibbon added a commit that referenced this pull request Nov 14, 2022
gmcgibbon added a commit that referenced this pull request Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants