Defer route drawing to the first request, or when url_helpers called

[gmcgibbon]

Motivation / Background

This Pull Request has been created because apps with lots of routes take a long time to boot. A developer could boot an app for reasons that don't involve routes at all (like running unit tests, migrations, rake tasks, etc.) so I think this should be deferred in dev and test.

Detail

This Pull Request changes engine and app route sets to a Rails::Engine::RouteSet, which knows about the current Rails application. The default middleware stack has also changed to include a Rails::Rack::LoadRoutes middleware that loads routes if needed. This PR loads routes under the following circumstances:

In dev/test:

• The first request via middleware
• When application or engine url_helpers.some_path is called via method_missing?
• When application or engine url_helpers.respond_to?(:some_path) is called via respond_to_missing?

In production:

• In the finisher eagerly, which is the previous behaviour

If, for some reason, a developer wishes to revert to the previous behaviour, they could add an initializer with Rails.application.reload_routes!. However, if we wanted to be more safe, we could also hide this behaviour behind a configuration variable.

Additional information

This is loosely inspired by https://github.com/amatsuda/routes_lazy_routes

I'm a bit concerned this is too much of a hack, but I think the speed benefit might be worth it. In an app with 2000 routes:

Before:

% time bin/rails runner ""
bin/rails runner ""  0.66s user 0.33s system 95% cpu 1.036 total

After:

% time bin/rails runner ""
bin/rails runner ""  0.47s user 0.32s system 98% cpu 0.791 total

Checklist

Before submitting the PR make sure the following are checked:

• [x] This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
• [x] Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• [x] Tests are added or updated if you fix a bug or add a feature.
• [x] CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[gmcgibbon]

Should be green now. I'm using method_missing and middleware to load routes instead of doing it eagerly, and it seems to work pretty well, but I'd like some feedback before I merge.

[gmcgibbon]

cc @casperisfine since you had opinions about the gem that did this. Does this seem like an acceptable approach to you?

[gmcgibbon]

I think this might break routing test helpers in Rails where applications draw test routes. The middleware may break that, so I'll check before merging.

[casperisfine]

Does this seem like an acceptable approach to you?

I only had a quick look, but yes, seem good to me.

[Fryguy]

I opened an issue along time ago about how the removal of dynamic actions caused our number of routes to explode, and used a ton more memory: https://github.com/rails/rails/issues/27231. Since then we had switch to routes_lazy_routes, but I would prefer to remove that gem and have it baked in, so I'm looking forward to this PR.

[jeromedalbert]

FYI this PR seems to have broken rspec-rails controller tests. Since it was merged very recently I wasn't sure where to report this (here, in a new issue, or in a rspec-rails issue). I decided to submit an issue to rspec-rails and provide the link here: https://github.com/rspec/rspec-rails/issues/2761.

[rosa]

It seems this also breaks URL helpers in helper tests that extend ActionView::TestCase, as route drawing hasn't happened and any URL helper is undefined. Not sure if it's intended. For now I've solved this for our specific test with:

  setup do
    routes_reloader = Rails.application.routes_reloader
    routes_reloader.execute_unless_loaded if routes_reloader&.respond_to?(:execute_unless_loaded)
  end

[rafaelfranca]

Ok. I'm going to revert this PR from 7.2 since it is too close to the release and there are too many unknown issues. Can you open an issue @rosa, please?

[rosa]

@rafaelfranca thanks! I haven't been able to create a minimal reproduction script, though 😕 Do you still want an issue without that?

[rafaelfranca]

Yes. That would allow us to not forget to solve the problem.

[gmcgibbon]

Looks like I missed a few use-cases. Sorry folks, I'm working on a followup patch this week. Thanks for reverting!