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

Zeitwerk integration #35235

Merged
merged 2 commits into from Feb 12, 2019

Conversation

Projects
None yet
4 participants
@fxn
Copy link
Member

fxn commented Feb 12, 2019

Introduction

This patch integrates Zeitwerk and makes it the default autoloader for Rails 6 applications.

  • You can now robustly use constant paths in class and module definitions:

    # Matches Ruby semantics now.
    class Admin::UsersController < ApplicationController
      # ...
    end
  • All known use cases of require_dependency have been eliminated.

  • There is a centralized solution to orderly preload STI hierarchies.

  • Edge cases in which the autoloaded constant depended on execution order are gone.

  • Autoloading is thread-safe in general, not just for the currently supported use cases with explicit locks like web requests. For example, you can now write multi-threaded scripts to be run by bin/rails runner and they will autoload just fine.

Principles behind the integration

The classic autoloader has been there since the beginning, and while it has no public interface except for locking mechanisms, extra care has been taken:

  • The implementation of ActiveSupport::Dependencies and its test suite are untouched.

  • Easy way to opt-out: just throw config.autoloader = :classic into config/application.rb. It has to go after config.load_defaults 6.0 if you have that line.

  • Integration has been clearly delineated so that falling back to the classic autoloader is safe.

  • It is a key observation that Kernel#autoload is invoked by the constant resolution algorithm as classes and modules are checked. In particular, it takes precedence over const_missing and thus we can leave const_missing there to have a cleaner patch, it won't do anything anyway for autoloadable constants.

  • Seamless take over in new apps from an end-user point of view.

I envision a gradual evolution of this feature, Rails 6 is just a first step.

Patch overview

  • There is a new config attribute config.autoloader that can be :classic or :zeitwerk. This is :zeitwerk for new apps on CRuby, and :classic for apps loading the defaults of previous versions or run by other interpreters.

  • If Zeitwerk is enabled, there are two globally accessible autoloaders: Rails.autoloader, and Rails.once_autoloader. The former is the main one. The latter, in addition to not win a Pulitzer for naming, is responsible for autoload_once_paths, and importantly also for autoload paths that come from gems. Both autoloaders autoload, but only Rails.autoloader is invoked by Rails to reload, so if you have engines like Action Storage or Devise installed as gems, they won't be reloaded anymore.

  • In particular, if you throw Rails.autoloader.logger = method(:puts) to config/application.rb you'll see Zeitwerk doing its thing.

  • Selected private API is emulated, but it is not the goal of the integration to fully replicate the private API. If there is code that depends on the inner workings excessively, I prefer that people switch to :classic until it gets updated, to clutter the integration to support any imaginable abuse of the private API. This autoloading is an inflection point and it is our opportunity to start afresh.

  • Furthermore, the private API called downwards from const_missing should not be called anymore because, as explained above, Kernel#autoload runs first. There might be some edge case around autoloading errors, not worth it, keep it simple.

  • People should not write require_dependency anymore, and upgrades should delete the calls. If you detect a use case for it please tell us.

  • Eager loading calls Zeitwerk.eager_load_all. Therefore, it will eager load not only the Rails application, but also all gems managed by Zeitwerk.

  • Engines can technically use Zeitwerk by clearing their autoload paths, but better don't and leave the choice to the parent application.

  • Autoload paths are frozen in the finisher. The public API for them is config.autoload_paths, but if existing code is pushing to ActiveSupport::Dependencies.autoload_paths, it has to do so before the finisher. Action Mailer has been updated to do that.

  • Applications using bootsnap have to upgrade to 1.4.0.

Backwards incompatibility

In :classic mode there is no change. If Zeitwerk is enabled:

  • For files below the standard concerns directories (like app/models/concerns), Concerns cannot be a namespace. That is, app/models/concerns/geolocatable.rb is expected to define Geolocatable, not Concerns::Geolocatable.

  • A file should define only one constant in its namespace (but can define inner ones). So, if app/models/foo.rb defines Foo and also Bar, Bar won't be reloaded cleanly. You can have inner constants, so Foo may define an auxiliary internal class Foo::Woo.

  • A file that defines a class or module that acts as a namespace, needs to define the class or module using the class and module keywords. For example, if you have app/models/hotel.rb defining the Hotel class, and app/models/hotel/pricing.rb defining a mixin for hotels, the Hotel class must be defined with class, you cannot do Hotel = Class.new { ... } or Hotel = Struct.new { ... } or anything like that. Those idioms are fine in classes and modules that do not act as namespaces.

  • Once the application has booted, autoload paths are frozen.

  • Autoload paths that do not exist on boot are ignored.

Future work

I believe the patch is in good shape for a beta. For final, I'll work on documentation. In particular, the Autoloading and Reloading Constants needs a complete revision.

There are some if RUBY_ENGINE == "ruby" and config.autoloader == :zeitwerk here and there, I'll try to encapsulate them somehow.

@tenderlove
Copy link
Member

tenderlove left a comment

omg this patch is amazing!! Great job!

if options.show_previews && options.preview_path
ActiveSupport::Dependencies.autoload_paths << options.preview_path
end
end

This comment has been minimized.

@tenderlove

tenderlove Feb 12, 2019

Member

I really like that setting autoload paths is split from the other initializer. 👍

@tenderlove

This comment has been minimized.

Copy link
Member

tenderlove commented Feb 12, 2019

  • A file should define only one constant in its namespace (but can define inner ones). So, if app/models/foo.rb defines Foo and also Bar, Bar won't be reloaded cleanly.

Is this classic mode behavior, or Zeitwerk behavior? I thought it wouldn't be reloaded cleanly in classic mode.

@fxn

This comment has been minimized.

Copy link
Member Author

fxn commented Feb 12, 2019

  • A file should define only one constant in its namespace (but can define inner ones). So, if app/models/foo.rb defines Foo and also Bar, Bar won't be reloaded cleanly.

Is this classic mode behavior, or Zeitwerk behavior? I thought it wouldn't be reloaded cleanly in classic mode.

In classic mode, if you define Foo = Bar = 1 in app/models/foo.rb, dependencies has Bar in autoloaded_constants. I believe that is a side-effect of the implementation, more than a deliberate feature.

In the new mode we enforce one file = one constant path, except for inner classes. There is a technical reason for it, which is that we keep track of what is loaded in the decoration of Kernel#require. Given an absolute file name, we know the parent module and constant name that corresponds to, there is no way to detect Bar.

@fxn fxn merged commit ed9acb4 into master Feb 12, 2019

3 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@fxn fxn deleted the fxn/zeitwerk branch Feb 12, 2019

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Feb 13, 2019

  • Autoload paths that do not exist on boot are ignored.

Can you expand on that one?

Is that true even after a reload? Because that sounds Bad.

@matthewd
Copy link
Member

matthewd left a comment

Should zietwerk be an explicit dependency of activesupport?

It seems unfortunate to require both a config setting and a Gemfile line. If we do keep it like this, I suspect there's at least room for improvement on the error-reporting front.

(Correspondingly, I note no require "zeitwerk" anywhere other than the test app.)

gem "bootsnap", ">= 1.1.0", require: false
gem "bootsnap", ">= 1.4.0", require: false

gem "zeitwerk", ">= 1.0.0" if RUBY_ENGINE == "ruby"

This comment has been minimized.

@matthewd

matthewd Feb 13, 2019

Member

Need platform: here; if and Gemfiles don't mix well

This comment has been minimized.

@fxn

fxn Feb 13, 2019

Author Member

Oh yes.


<%- end -%>
<%- if options.api? -%>
# Use Rack CORS for handling Cross-Origin Resource Sharing (CORS), making cross-origin AJAX possible
# gem 'rack-cors'

<%- end -%>
<% if RUBY_ENGINE == 'ruby' -%>
<% if RUBY_ENGINE == "ruby" -%>
gem "zeitwerk", ">= 1.0.0"

This comment has been minimized.

@matthewd

matthewd Feb 13, 2019

Member

All the rest of this file uses single quotes

This comment has been minimized.

@fxn

fxn Feb 13, 2019

Author Member

But our style guidelines have changed over time, saw an opportunity to update that one I was touching.

True that the file should be consistent.

@fxn

This comment has been minimized.

Copy link
Member Author

fxn commented Feb 13, 2019

Should zietwerk be an explicit dependency of activesupport?

Being a dependency would be fine too. Maybe better in the sense that it is a dependency, but if everything goes well one that is going to gradually replace the classic autoloader, it sends a stronger message.

I'll change this (unless someone has an objection).

@fxn

This comment has been minimized.

Copy link
Member Author

fxn commented Feb 13, 2019

  • Autoload paths that do not exist on boot are ignored.

Can you expand on that one?

Is that true even after a reload? Because that sounds Bad.

That is a design decision, but open to debate of course.

In Zeitwerk, pushing a non-existing directory is an error. As always, microdecisions. In this case the idea is to prevent misconfiguring directories with typos in their names or whatever and fail fast. On the other hand, the point of view in which you do not consider that to be an error, and just ignore the non-existing ones on setup, is also valid.

The decision depends on use cases. You can have empty directories, but not non-existing directories. Do we have real use cases that deserve changing this logic? Similarly, with classic autoloading, the subdirectories under app at boot are the ones in autoload paths, if you add a new one (app/services), you need to restart the server.

@eregon

This comment has been minimized.

Copy link

eregon commented Feb 13, 2019

  • There is a new config attribute config.autoloader that can be :classic or :zeitwerk. This is :zeitwerk for new apps on CRuby, and :classic for apps loading the defaults of previous versions or run by other interpreters.

@fxn Any reason for it to be only enabled on CRuby by default?

@fxn

This comment has been minimized.

Copy link
Member Author

fxn commented Feb 13, 2019

@fxn Any reason for it to be only enabled on CRuby by default?

Hey @eregon! It was in my TODO to contact the JRuby/TruffleRuby teams about it.

Right now, Zeitwerk only supports CRuby. One of the reasons is that it needs Kernel#autoload to call Kernel#require, because it is key to be able to decorate it. I am watching jruby/jruby#5403 regarding this.

Another detail, is that in order to support what I call explicit namespaces the library sets a trace point on the :class event. Last time I checked, JRuby does not emit this particular event unless you pass the --debug flag. I am not familiar with that flag, but does not seem like something you'd enable in production normally.

On the other hand the :class event is triggered rarely in regular code bases, so I wanted to ask if the JRuby team would consider broadcasting that one in the default mode.

Furthermore, the test suite has a Ruby compatibility suite in case it helps. I don't know right now if it is exhaustive, should do a pass and document in which way we depend on each property, but it is a good indicator.

Please anything else just let me know, would be awesome that the library is portable.

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Feb 13, 2019

Do we have real use cases that deserve changing this logic? Similarly, with classic autoloading, the subdirectories under app at boot are the ones in autoload paths, if you add a new one (app/services), you need to restart the server.

I strongly feel that's a bug worth fixing, not doubling down on... and I think it's hugely more common to create a new namespaced model (that is, a new namespace) than it is to invent a new app/*/ type, so I think this will be quickly problematic if I'm correctly understanding the new behaviour.

To be clear, I agree with the design decision at Zeitwerk's level that a missing directory should be an error: I just think that Rails in its soft-friendly-autoloady configuration will want to be more forgiving, and I think that means a fully dynamic app (drawing a distinction between "basic" autoload and "genuinely expect files to change/appear/disappear" reload+autoload) ultimately can't freeze the path list.

(Zeitwerk doesn't need to react to such directories appearing, of course -- our file watcher will handle that -- it just needs to be open to having its paths reconfigured during the reload cycle.)

@fxn

This comment has been minimized.

Copy link
Member Author

fxn commented Feb 13, 2019

I think it's hugely more common to create a new namespaced model (that is, a new namespace) than it is to invent a new app/*/ type, so I think this will be quickly problematic if I'm correctly understanding the new behaviour.

Ah, let's make sure we are in sync on this one! New namespaces are picked up just fine. If you create app/models/admin and reload, Admin is autoloadable right away. The only thing we ignore are paths in ActiveSupport::Dependencies.autoload_paths that by the time the finisher runs do not exist (code).

@fxn

This comment has been minimized.

Copy link
Member Author

fxn commented Feb 13, 2019

@matthewd now that we talk about this, I believe that I wrote that if influenced by the current tolerance to non-existing directories. But on a second thought, I wonder if we should also err in the new mode instead of silently ignoring the autoload path.

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Feb 13, 2019

Ah okay, I misunderstood your previous wording, but I do still think we should support that too. (I didn't actually think it was currently so insensitive to new appearances (modulo complications from spring)

Loosely related: #33822

fxn added a commit that referenced this pull request Feb 15, 2019

Replace autoloader accessors with Rails.autoloaders.{main,once}
Rails.autoloader and Rails.once_autoloader was just tentative API good
enough for a first patch. Rails.autoloader is singular and does not
convey in its name that there is another autoloader. That might be
confusing, for example if you set a logger and miss traces. On the other
hand, the name `once_autoloader` is very close to being horrible.

Rails.autoloaders.main and Rails.autoloaders.once read better for my
taste, and have a nice symmetry. Also, both "main" and "once" are four
letters long, short and same length.

They are tagged as "rails.main" and "rails.once", respectively.

References #35235.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.