Skip to content

Conversation

@jorgemanrubia
Copy link

@jorgemanrubia jorgemanrubia commented Nov 1, 2024

This supports optionally configuring zeitwerk when creating new gems.

I think Zeitwerk significantly improves the development ergonomics when loading Ruby classes. If you follow its file structure conventions, you don't have to manually require files in your gem. Zeitwerk is fast, robust and has been battle-tested in Rails and many other popular gems.

This is a subjective territory, but I think using Zeitwerk in fresh gems is a no-brainer: if you follow its conventions—which are those of Ruby—you fully remove a tricky concern from your plate: manually building the requires tree. Following conventions and making your life easier benefit both newcomers and advanced users.

This will add a new question to configure Zeitwerk when creating new gems (false by default).

Do you want to use Zeitwerk to load classes?
With Zeitwerk (https://github.com/fxn/zeitwerk), Ruby can load classes automatically based on name conventions so that you don't have to require files manually. y/(n):

It also supports a --zeitwerk flag to configure Zeitwerk directly:

bundle gem mygem --zeitwerk

cc @fxn

@welcome
Copy link

welcome bot commented Nov 1, 2024

Thanks for opening a pull request and helping make RubyGems and Bundler better! Someone from the RubyGems team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use GitHub Actions to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of GitHub Actions in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #rubygems or #bundler channel on Slack.

For more information about contributing to the RubyGems project feel free to review our CONTRIBUTING guide

@jorgemanrubia jorgemanrubia changed the title Add option to optionally configure zeitwerk in new gems Add option to configure zeitwerk in new gems Nov 1, 2024
silva96

This comment was marked as resolved.

@deivid-rodriguez
Copy link
Contributor

deivid-rodriguez commented Nov 1, 2024

I'm a big fan of zeitwerk, and I consider bundle gem essentially community driven, so I'm good with this!

One argument against is additional maintenance. Eventually we may want to offload maintenance from core and move bundle gem to a plugin, but for now it does not cause excessive work and it's in practice maintained by external contributors.

Of course, let's see what other maintainers think, though!

"With Zeitwerk (https://github.com/fxn/zeitwerk), Ruby can load classes automatically " \
"based on name conventions so that you don't have to require files manually.")
config[:zeitwerk] = true
end
Copy link
Contributor

@jeromedalbert jeromedalbert Nov 1, 2024

Choose a reason for hiding this comment

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

Should we ask for this, which would set a BUNDLE_GEM__ZEITWERK config setting that will be remembered across bundle gem calls?

Maybe :zeitwerk is one of those options like :exe that could be left as one-off options. While one may want zeitwerk sometimes (or often), I don't know if it is a setting that should be remembered as if you are doing a very simple gem with 2-3 files, it's easy enough to require manually and one might want to forego zeitwerk to avoid a third-party dependency in their gem. But because they could have enabled zeitwerk before, zeitwerk would be enabled for their new small gem too. I guess in this case one could always provide --no-zeitwerk, so maybe this is fine as-is.

Just thought I would comment in case anyone has opinions on this.

Copy link
Author

Choose a reason for hiding this comment

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

@jeromedalbert I went with the same approach it uses for the CoC or the MIT license, which results in question + --zeitwerk/--no-zeitwerk option + memorized answer in ~/.bundle/config. I personally think it's fine and consistent but happy to change it if you want me to.

@byroot
Copy link
Member

byroot commented Nov 1, 2024

What about eager loading though? By default Zeitwerk lazy load, which is great for development, but in production you absolutely want eager loading.

For Rails applications this is fine because Rails has a notion of production environment, and in this case will automatically call Zeitwerk::Loader.eager_load_all. But users other than Rails apps don't have such a clear definition of when lazy loading is undesirable.

@jorgemanrubia jorgemanrubia force-pushed the configure-zeitwerk branch 2 times, most recently from 9179256 to 68f55e2 Compare November 1, 2024 18:15
@jorgemanrubia
Copy link
Author

What about eager loading though? By default Zeitwerk lazy load, which is great for development, but in production you absolutely want eager loading.

That's a good point @byroot. Maybe we could add a comment about the class-loading situation to the generated README?

Do you have thoughts here @fxn?

@jeromedalbert
Copy link
Contributor

jeromedalbert commented Nov 1, 2024

Maybe we could add a comment about the class-loading situation to the generated README?

Another option could be to add a # loader.eager_load comment at the end of newgem.rb.tt, similar to https://github.com/fxn/zeitwerk#synopsis, something like

# Uncomment the line below if you want to eager load
# loader.eager_load

@fxn
Copy link
Contributor

fxn commented Nov 1, 2024

My thoughts:

  1. In general, I believe people prefer their code to be lazy loaded by default:
    • This is good for fast tests workflows.
    • If the gem is a dependency of a Rails app, it contributes to reduce boot time in dev/test modes.
  2. As @byroot said, the gem author may not know in advance if it is good to eager load or not.

So, lazy by default, and if client code responsible for runtime performance is in a situation in which eager loading is better, that client code has API to eager load anything managed by Zeitwerk.

Generating the comment suggested by @jeromedalbert could be a compromise, though that could make people wonder and uncomment "just in case", defeating some of the goals above.

<%- if config[:name].include?("-") -%>
loader = Zeitwerk::Loader.for_gem_extension(<%= config[:constant_array][0..-2].join("::") %>)
<%- else -%>
loader = Zeitwerk::Loader.for_gem
Copy link
Contributor

Choose a reason for hiding this comment

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

I always wondered why the local var. Any drawbacks in doing

Suggested change
loader = Zeitwerk::Loader.for_gem
Zeitwerk::Loader.for_gem.setup

@simi
Copy link
Contributor

simi commented Nov 1, 2024

Does this play well with native extensions also? Is there any limitation on that?

@byroot
Copy link
Member

byroot commented Nov 2, 2024

So, lazy by default

I'd be more comfortable with eager by default, and lazy in development only. Of course the issue being how to signal that?

Because with lazy by default there is also the risk that the author doesn't realize it needs some eager load exclusions (typically if the gem contains an adapter pattern like ActiveRecord::ConnectionAdapters).

I really want to stress out that lazy loading in production is really terrible for performance, as it causes massive latency spikes around deploys, and waste a lot of memory by not loading code before fork (assuming a pre-forking server).

@jorgemanrubia jorgemanrubia force-pushed the configure-zeitwerk branch 3 times, most recently from 48c48d4 to b88bfb8 Compare November 2, 2024 08:14
@fxn
Copy link
Contributor

fxn commented Nov 2, 2024

I believe we cannot talk about "modes", because this is arbitrary gems.

However, in CI it makes sense to eager load. You want to eager load because if CI does not exercise a file and the file is malformed or does not define the expected constant, you want to know.

So, it could be

loader.eager_load if ENV.key?('CI')

Still, eager load exclusions are on you if you need them. If you develop with all connector dependencies available (very likely in dev and test) you may not realize you need exclusions even if you eager load. But I can't think of a way to automate preventing that, it is gem semantics. While subtle, in this case I cannot think of an alternative to "the user has to be responsible for using the tool correctly". Worse case scenario is that you have a "bug" if you forget that, and need to ship a fix.

@byroot
Copy link
Member

byroot commented Nov 2, 2024

Thing is, I suspect most gems are small enough that lazy loading doesn't make a big difference. So I wonder if just defaulting to eager loading doesn't make more sense? Then it's up to the gem to offer an eager loading mechanism if they wish to default to lazy loading.

@fxn
Copy link
Contributor

fxn commented Nov 2, 2024

Thing is, I suspect most gems are small enough that lazy loading doesn't make a big difference.

Agree at the individual level. And if you are not concerned about the potential impact of this being a default in Rails applications boot time, neither I am :). However, consider the compound effect when 30 dependencies of your project eager load at boot time.

This default represents a trade-off, I am sharing my point of view, but ultimately I think the Ruby and RubyGems teams have to decide. I'm fine either way :).

@byroot
Copy link
Member

byroot commented Nov 2, 2024

However, consider the compound effect when 30 dependencies of your project eager load at boot time.

To be clear, in the context of a Rails app, having your dependencies using Zeitwerk in lazy loading mode is somewhat a win-win as it means faster development boot time, and perfect eager loading in production.

It's really usage outside of Rails for which I'm more worried. e.g. Sinatra apps and such.

@fxn
Copy link
Contributor

fxn commented Nov 2, 2024

Generally speaking, I believe parent applications are responsible for that kind of performance tweaks. Parent applications are the ones that know what is more suitable in a given runtime.

In that line, I believe client code is the one that should say "I'll have spikes or insufficient CoW gains if I don't eager load, let's do it". And they should ideally use the eager loading API on behalf of their users.

@deivid-rodriguez
Copy link
Contributor

I don't have a strong opinion either way but I tend to think lazy load is a better default for fresh gems that only exist on a single user machine and have not yet been pushed to a gem registry, let alone used in production.

Alternatively, we could provide two different flags and let users decide each time, like --zeitwerk-lazy and --zeitwerk-eager, but I feel most users won't care or want to even think about this in such an early stage.

@simi
Copy link
Contributor

simi commented Nov 4, 2024

Alternatively, we could provide two different flags and let users decide each time, like --zeitwerk-lazy and --zeitwerk-eager, but I feel most users won't care or want to even think about this in such an early stage.

maybe --zeitwerk=lazy, --zeitwerk=eager 🤔

@simi
Copy link
Contributor

simi commented Nov 4, 2024

One argument against is additional maintenance. Eventually we may want to offload maintenance from core and move bundle gem to a plugin, but for now it does not cause excessive work and it's in practice maintained by external contributors.

I think we should keep it, but make it possible to onboard external templates like Rails does.

@jeromedalbert
Copy link
Contributor

jeromedalbert commented Nov 4, 2024

I feel most users won't care or want to even think about this in such an early stage.

Agreed, when a gem gets to a later stage the user can always read up on Zeitwerk's README and configure eager loading themselves if needed, fully or partially to make some setups faster (example). In the meantime the gem generator gives you the basic plumbing necessary to quickly get you started with Zeitwerk.

On the other hand users may never know that Zeitwerk eager loading is a thing, so they might benefit from a mention somewhere somehow, whether in a Readme, or comment, or seeing --zeitwerk=lazy|eager in bundle gem's help, or some other way. Although I would argue that if users are interested in enabling Zeitwerk in the first place they would probably have read up on it, at least partially, especially if the --zeitwerk option were not asked by ask_and_set during gem creation, meaning that they would have gone out of their way to enable it.

@byroot
Copy link
Member

byroot commented Nov 4, 2024

Agreed, when a gem gets to a later stage the user can always read up on Zeitwerk's README and configure eager loading themselves if needed,

I'd argue that it's the opposite. When the gem gets bigger it becomes interesting to lazy load it in development, but because that it doesn't bring much of anything for development but is detrimental for production.

@deivid-rodriguez
Copy link
Contributor

If Zeitwerk's author and Zeitwerk's contributor #2 don't agree on what the default should be, maybe that means we should have a flag with two values (--zeitwerk=lazy and --zeitwerk=eager), and no default?

@byroot
Copy link
Member

byroot commented Nov 6, 2024

@deivid-rodriguez the problem isn't so much of defining a default, the problem is that the ideal state is to get both lazy loading in development and eager loading in production. But that's a concept that only defined by the final application or frameworks like Rails.

As mentioned previously, I honestly don't mind if Zeitwerk is configured in lazy loading mode, because Rails will force eager loading of all Zeitwerk enabled gems anyway. So selfishly, it doesn't change anything for me, and Rails users will get the best of both world.

But for users of other web frameworks that will provide a bad production experience, with latency spikes on deploy, and increased memory usage, unless those frameworks or applications are Zeitwerk aware.

But perhaps the "solution" is to default to lazy loading, and add a note in the generated README on how to eager load in production.

@deivid-rodriguez
Copy link
Contributor

Then should those other frameworks do the same as Rails?

My idea by not defining a default is to make sure interested users evaluate beforehand what makes more sense for the gem they are about to create.

@byroot
Copy link
Member

byroot commented Nov 6, 2024

Then should those other frameworks do the same as Rails?

If they wish to be Zeitwerk aware then sure, but they may not want to? If the framework isn't doing it, then it fall onto the user to know to call Zeitwerk::Loader.eager_load_all in production.

The only issue is that if I add gem XXX, it's not obvious I have to do that, unless of course it's documented in the readme (e.g. as part of the bundle add XXX autogenerated documentation).

@byroot
Copy link
Member

byroot commented Nov 6, 2024

to make sure interested users evaluate beforehand what makes more sense for the gem they are about to create.

Why I tried to explain is that what makes sense doesn't depend on the gem, both makes sense for all gems. What define which one you want is in which environment you run the gem. So the author can't decide for the user.

@deivid-rodriguez
Copy link
Contributor

I see. Then I guess the README note is a good idea?

We can provide both options, and also add a README note next to bundle add that explains what loading will be like when the gem is added and how the loading behavior can be tweaked?

@jeromedalbert
Copy link
Contributor

jeromedalbert commented Nov 6, 2024

@byroot I am not super familiar with Zeitwerk besides playing around with it a few times in some gems, so forgive me for the naive question, but when you talk about wanting lazy loading for development and eager loading for production, to me "development" and "production" are terms usually used for web applications like Rails or maybe applications that run continuously on some machine, unless I am wrong. So what do you think the Zeitwerk setup should be for gems that don't fit these kinds of applications, like CLIs and other one-off tools? Since I believe the bundle gem generator should cater to all kinds of gems.

@byroot
Copy link
Member

byroot commented Nov 6, 2024

are terms usually used for web applications like Rails or maybe applications that run continuously on some machine

Yes. There are cases like CLIs where indeed, lazy loading can be preferable.

But the point remain, it's the final user of the gem (as in the final application) that need to decide which is desirable, so aside from a few exceptions, a gem can't know in which context it will be used and can't decide between eager loading and lazy loading.

@fxn
Copy link
Contributor

fxn commented Nov 7, 2024

Agree with all the points made by Jean.

It's one of those situations in which a trade-off has to be made and move on.

If the default is not optimal for a given gem, well, they are using Zeitwerk, so they should configure it to their needs anyway. We have to follow our hunch for what we believe makes sense most of the time.

Nowadays, gems do not even have the option, at least not so easily and with a dual mode lazy/eager available. Unless they do something sophisticated like Rails components do. I'd say that, statistically, most gems today eager load as a side-effect of nested require calls initiated at their entrypoint, because doing anything else is more complicated without an autoloader.

@fxn
Copy link
Contributor

fxn commented Nov 7, 2024

If we go lazy by default, I'd generate the CI line at the bottom (off-the-top of my head, feel free to edit):

# Client code may eager load the gem, make sure that works.
# If some files or directories should never be eager loaded,
# please configure eager load exceptions in the loader.
loader.eager_load if ENV.key?('CI')

Before we did this in Rails (eager loading by default in the test environment if in CI), occasionally people pushed code to production that was not even loaded by the test suite (much less tested) and was incorrect. Without Zeitwerk, if a file is not loaded, it could even be malformed. But with that line we may help preventing this, raise awareness of the fact that client code can eager load you, and since we are on it also mention eager load exceptions for the same price.

@deivid-rodriguez
Copy link
Contributor

So if I'm getting this right, we're leaning towards lazy by default rather than providing two separate option values, and no default. Correct?

@byroot
Copy link
Member

byroot commented Nov 7, 2024

Yeah, I think lazy loading with a README note about what it means / what to do is probably the way to go.

@jorgemanrubia
Copy link
Author

Yeah, I think lazy loading with a README note about what it means / what to do is probably the way to go.

Ok, I'll add the README note when I have a chance this week 👍 . Thanks for the feedback folks.

@jorgemanrubia jorgemanrubia force-pushed the configure-zeitwerk branch 2 times, most recently from 0ba05e2 to 43fa181 Compare November 17, 2024 10:23
Add a question to configure Zeitwerk when creating new gems (false by default). It
also supports a --zeitwerk flag to do it:

    bundle gem mygem --zeitwerk
@jorgemanrubia
Copy link
Author

Hey folks, I pushed some changes based on the discussion above.

  1. It includes this line next to the loader setup code, as suggested by @fxn:
# Client code may eager load the gem, make sure that works.
# If some files or directories should never be eager loaded,
# please configure eager load exceptions in the loader.
loader.eager_load if ENV.key?('CI')
  1. It adds this entry to the README, nested under ## Installation:

Zeitwerk and class loading

This gem uses Zeitwerk, which, by default, loads classes lazily as they are referenced in the code. In production environments, it's common to load code eagerly for performance reasons. If you're running this gem in a context that supports Zeitwerk—such as Rails or Hanami—no additional configuration is necessary. Otherwise, you may want to eager load this gem.

@fxn
Copy link
Contributor

fxn commented Nov 17, 2024

@jorgemanrubia I am not sure about the section in the README. Let me explain.

On one hand, the fact that a gem uses Zeitwerk has always been considered internal, it does not belong to the public interface. In your Gemfile, you do not know who loads how. And that is by design.

On the other hand, client code has no public interface to access the gem's loader. It cannot grab the loader and invoke eager_load on it. That is aligned with the design goal above.

If client code wants to eager load its dependencies using Zeitwerk, as Rails does, it has API for it that does not depend on who is loading with Zeitwerk. BTW, I believe that Hanami eager loads application slices, but I am not sure it invokes Zeitwerk::Loader.eager_load_all.

@ruby ruby deleted a comment from doaamegehahed Nov 18, 2024
@jorgemanrubia
Copy link
Author

If client code wants to eager load its dependencies using Zeitwerk, as Rails does, it has API for it that does not depend on who is loading with Zeitwerk. BTW, I believe that Hanami eager loads application slices, but I am not sure it invokes Zeitwerk::Loader.eager_load_all.

@fxn would you rephrase the README section or just remove it completely? Do you mean that maybe we should link to the documentation for .eager_load_all that does not depend on Zetiwerk internals? If you want to take a stab at the copy please go ahead.

@fxn
Copy link
Contributor

fxn commented Nov 18, 2024

@jorgemanrubia I would not mention Zeitwerk in the README of the gem.

@johnnyshields
Copy link
Contributor

johnnyshields commented Jan 2, 2025

My two cents (apologies for bike-shedding) -- lazy-load makes sense, but it would be good if the generated code could have a test by default which calls Zeitwerk.eager_load_all and verifies it does not raise any error.

@martinemde
Copy link
Contributor

It looks like we're just waiting on an update to the readme template and then this could merge. A lot of effort was put into resolving the right choices here and yet it has stalled with a tiny change remaining.

Is there anything else that is missing?

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.

10 participants