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

Disable config.add_autoload_paths_to_load_path by default in Rails 7.1 #44133

Merged
merged 1 commit into from Jan 11, 2022

Conversation

casperisfine
Copy link
Contributor

Ref: 668673f

Now that Zeitwerk is the only autoloader, there no reason to add autoloaded paths to $LOAD_PATH (we should probably have done it in 7.0, but hey...).

@fxn let me know if it's good with you and I can merge.

@fxn
Copy link
Member

fxn commented Jan 10, 2022

Oh yes, let's do this.

One detail, it's a little secret that

which would cause reloading issues

is actually not the case due to a undocumented and experimental hack written for a different use case.

What we could say is that you just don't use require with autoloadable code, that is the whole point, just reference the constant.

@@ -261,6 +261,8 @@ def load_defaults(target_version)
when "7.1"
load_defaults "7.0"

self.add_autoload_paths_to_load_path = false
Copy link
Member

Choose a reason for hiding this comment

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

Update the configuring guide 😂

Copy link
Member

Choose a reason for hiding this comment

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

Arf, everytime I miss one -_-

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it's already documented.

Copy link
Member

Choose a reason for hiding this comment

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

I mean document that in 7.1 the default will be false. But I guess we need to merge #44120 first.

Ref: rails@668673f

Now that Zeitwerk is the only autoloader, there no reason
to add autoloaded paths to `$LOAD_PATH`.
@byroot byroot merged commit b6987d6 into rails:main Jan 11, 2022
@fxn
Copy link
Member

fxn commented Jan 11, 2022

I suppose, at some point we could just drop all this. Can't think of any use case for require + autoload path. The few that exist are covered with require_dependency.

@casperisfine
Copy link
Contributor Author

Absolutely. We'll just have to leave it for at least 1 version, and maybe find a way to deprecate it?

@fxn
Copy link
Member

fxn commented Jan 11, 2022

👍

@yahonda
Copy link
Member

yahonda commented Jan 11, 2022

As you may be already aware that, would you update these tests also?

cd railties
bin/test test/application/configuration_test.rb:1833 # test_autoload_paths_are_added_to_$LOAD_PATH_by_default 
bin/test test/application/configuration_test.rb:1872 # test_autoload_paths_can_be_set_in_the_config_file_of_the_environment
bin/test test/application/initializers/load_path_test.rb:24 # test_initializing_an_application_adds_the_application_paths_to_the_load_path
bin/test test/application/paths_test.rb:72 # test_load_path_includes_each_of_the_paths_in_config.paths_as_long_as_the_directories_exist

@byroot
Copy link
Member

byroot commented Jan 11, 2022

As you may be already aware

No, I'm just a big idiot. Sorry about that one... I'll fix it ASAP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants