Only execute route reloads once on boot for development environment #40742
Conversation
dotLou
force-pushed
the
development_only_execute_route_reloader_once
branch
from
252341b
to
1680429
Compare
dotLou
force-pushed
the
development_only_execute_route_reloader_once
branch
from
1680429
to
dcf5f5e
Compare
| paths.each { |path| load(path) } | ||
| run_callbacks :load_paths do | ||
| paths.each { |path| load(path) } | ||
| end |
There was a problem hiding this comment.
Do we need the callback setup feels like we could just do this?
def load_paths
paths.each { |path| load(path) }
after_load_path_callback&.call
endThere was a problem hiding this comment.
@rafaelfranca had suggested this but it might very well be faster not to use the activesupport callbacks? I have no strong feelings either way and am happy to make changes
There was a problem hiding this comment.
I'll remove it after the merge
| @@ -173,6 +173,22 @@ def self.complete(_state) | |||
| end | |||
| end | |||
|
|
|||
| initializer :add_builtin_route do |app| | |||
There was a problem hiding this comment.
| initializer :add_builtin_route do |app| | |
| initializer :add_internal_routes do |app| |
Can we rename this without anyone relying on the initializer name? Feels like we aren't adding just one more anymore and internal would be closer than builtin.
There was a problem hiding this comment.
I've gone ahead and made this change but will leave the comment unresolved if only so others can notice this change.
Signed-off-by: Louis Cloutier <louis.cloutier@shopify.com>
dotLou
force-pushed
the
development_only_execute_route_reloader_once
branch
from
dcf5f5e
to
796e712
Compare
|
@dotLou thank you, this was great 🙌 |
Signed-off-by: Louis Cloutier louis.cloutier@shopify.com
Summary
In #39980 a new supported use case was added to allow overriding of the welcome route in an app via
get '/'. Unfortunately, this introduced a doubleroutes_reloader.executein the development environment. For apps with lots of routes and/or lots of routes files, this creates a significant expense when booting in development environments.This PR aims to allow for that use case while only executing the routes reloader a single time. To do this, I've introduced a new callback that can be set via Proc on the routes reloader, and will be called after
load_paths.You can see that the number of items is decreased in the
LoadingTest, back to its original number, and the existing tests should continue to pass (and support overriding of the welcome route via bothrootandget '/'route definitions).Other Information
I've gone ahead and updated the changelog already, but please let me know if I should remove that or alter the text in any way.