From 33bf253282496072aa130de4cb49d0031cb3a37c Mon Sep 17 00:00:00 2001 From: Edouard CHIN Date: Thu, 5 Dec 2019 15:06:14 +0100 Subject: [PATCH] Bring back feature that allows loading external route iles: = This feature existed back in 2012 https://github.com/rails/rails/commit/5e7d6bba79393de0279917f93b82f3b7b176f4b5 but got reverted with the incentive that there was a better approach. After discussions, we agreed that it's a useful feature for apps that have a really large set of routes. Co-authored-by: Yehuda Katz --- actionpack/CHANGELOG.md | 23 ++++++++++++ .../lib/action_dispatch/routing/mapper.rb | 17 +++++++++ .../lib/action_dispatch/routing/route_set.rb | 3 +- guides/source/routing.md | 37 +++++++++++++++++++ .../lib/rails/application/routes_reloader.rb | 11 +++++- railties/lib/rails/engine.rb | 3 ++ railties/lib/rails/engine/configuration.rb | 1 + railties/lib/rails/paths.rb | 10 +++++ railties/test/application/paths_test.rb | 2 + railties/test/application/routing_test.rb | 36 +++++++++++++++++- 10 files changed, 138 insertions(+), 5 deletions(-) diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index f80b061755..b38911b0ea 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,26 @@ +* Bring back the feature that allows loading external route files from the router. + + This feature existed back in 2012 but got reverted with the incentive that + https://github.com/rails/routing_concerns was a better approach. Turned out + that this wasn't fully the case and loading external route files from the router + can be helpful for applications with a really large set of routes. + Without this feature, application needs to implement routes reloading + themselves and it's not straightforward. + + ```ruby + # config/routes.rb + + Rails.application.routes.draw do + draw(:admin) + end + + # config/routes/admin.rb + + get :foo, to: 'foo#bar' + ``` + + *Yehuda Katz*, *Edouard Chin* + * Fix system test driver option initialization for non-headless browsers. *glaszig* diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 6a5542bba5..575d9faf9d 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -1607,6 +1607,22 @@ module ActionDispatch !parent_resource.singleton? && @scope[:shallow] end + def draw(name) + path = @draw_paths.find do |_path| + File.exist? "#{_path}/#{name}.rb" + end + + unless path + msg = "Your router tried to #draw the external file #{name}.rb,\n" \ + "but the file was not found in:\n\n" + msg += @draw_paths.map { |_path| " * #{_path}" }.join("\n") + raise ArgumentError, msg + end + + route_path = "#{path}/#{name}.rb" + instance_eval(File.read(route_path), route_path.to_s) + end + # Matches a URL pattern to one or more routes. # For more information, see match[rdoc-ref:Base#match]. # @@ -2285,6 +2301,7 @@ module ActionDispatch def initialize(set) #:nodoc: @set = set + @draw_paths = set.draw_paths @scope = Scope.new(path_names: @set.resources_path_names) @concerns = {} end diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index 98cb58c47a..7b4a795f91 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -334,7 +334,7 @@ module ActionDispatch attr_accessor :formatter, :set, :named_routes, :default_scope, :router attr_accessor :disable_clear_and_finalize, :resources_path_names - attr_accessor :default_url_options + attr_accessor :default_url_options, :draw_paths attr_reader :env_key, :polymorphic_mappings alias :routes :set @@ -366,6 +366,7 @@ module ActionDispatch self.named_routes = NamedRouteCollection.new self.resources_path_names = self.class.default_resources_path_names self.default_url_options = {} + self.draw_paths = [] @config = config @append = [] diff --git a/guides/source/routing.md b/guides/source/routing.md index 3fe7b9a159..343304e5df 100644 --- a/guides/source/routing.md +++ b/guides/source/routing.md @@ -1165,6 +1165,43 @@ video = Video.find_by(identifier: "Roman-Holiday") edit_video_path(video) # => "/videos/Roman-Holiday/edit" ``` +Breaking up *very* large route file into multiple small ones: +------------------------------------------------------- + +If you work in a large application with thousands of routes, +a single `config/routes.rb` file can become cumbersome and hard to read. + +Rails offers a way to break a gigantic single `routes.rb` file into multiple small ones using the `draw` macro. + +```ruby +# config/routes.rb + +Rails.application.routes.draw do + get 'foo', to: 'foo#bar' + + draw(:admin) # Will load another route file located in `config/routes/admin.rb` +end + +# config/routes/admin.rb + +namespace :admin do + resources :comments +end +``` + +Calling `draw(:admin)` inside the `Rails.application.routes.draw` block itself will try to load a route +file that has the same name as the argument given (`admin.rb` in this case). +The file need to be located inside the `config/routes` directory or any sub-directory (i.e. `config/routes/admin.rb` , `config/routes/external/admin.rb`). + +You can use the normal routing DSL inside the `admin.rb` routing file, **however** you shouldn't surround it with the `Rails.application.routes.draw` block like you did in the main `config/routes.rb` file. + +### When to use and not use this feature + +Drawing routes from external files can be very useful to organise a large set of routes into multiple organised ones. You could have a `admin.rb` route that contains all the routes for the admin area, another `api.rb` file to route API related resources etc... + +However, you shouldn't abuse this feature as having too many route files make discoverability and understandability more difficult. Depending on the application, it might be easier for developers to have a single routing file even if you have few hundreds routes. You shouldn't try to create a new routing file for each category (admin, api ...) at all cost; the Rails routing DSL already offers a way to break routes in a organised manner with `namespaces` and `scopes`. + + Inspecting and Testing Routes ----------------------------- diff --git a/railties/lib/rails/application/routes_reloader.rb b/railties/lib/rails/application/routes_reloader.rb index 1070362253..3ba31f16c2 100644 --- a/railties/lib/rails/application/routes_reloader.rb +++ b/railties/lib/rails/application/routes_reloader.rb @@ -5,13 +5,14 @@ require "active_support/core_ext/module/delegation" module Rails class Application class RoutesReloader - attr_reader :route_sets, :paths + attr_reader :route_sets, :paths, :external_routes attr_accessor :eager_load delegate :execute_if_updated, :execute, :updated?, to: :updater def initialize @paths = [] @route_sets = [] + @external_routes = [] @eager_load = false end @@ -26,7 +27,13 @@ module Rails private def updater - @updater ||= ActiveSupport::FileUpdateChecker.new(paths) { reload! } + @updater ||= begin + dirs = @external_routes.each_with_object({}) do |dir, hash| + hash[dir.to_s] = %w(rb) + end + + ActiveSupport::FileUpdateChecker.new(paths, dirs) { reload! } + end end def clear! diff --git a/railties/lib/rails/engine.rb b/railties/lib/rails/engine.rb index 66dfff37b4..d7dc9869b0 100644 --- a/railties/lib/rails/engine.rb +++ b/railties/lib/rails/engine.rb @@ -593,10 +593,13 @@ module Rails initializer :add_routing_paths do |app| routing_paths = paths["config/routes.rb"].existent + external_paths = self.paths["config/routes"].paths + routes.draw_paths.concat(external_paths) if routes? || routing_paths.any? app.routes_reloader.paths.unshift(*routing_paths) app.routes_reloader.route_sets << routes + app.routes_reloader.external_routes.unshift(*external_paths) end end diff --git a/railties/lib/rails/engine/configuration.rb b/railties/lib/rails/engine/configuration.rb index ec16f8ba0d..3e9608245d 100644 --- a/railties/lib/rails/engine/configuration.rb +++ b/railties/lib/rails/engine/configuration.rb @@ -59,6 +59,7 @@ module Rails paths.add "config/initializers", glob: "**/*.rb" paths.add "config/locales", glob: "*.{rb,yml}" paths.add "config/routes.rb" + paths.add "config/routes", glob: "**/*.rb" paths.add "db" paths.add "db/migrate" diff --git a/railties/lib/rails/paths.rb b/railties/lib/rails/paths.rb index 0664338e0b..664e7b8832 100644 --- a/railties/lib/rails/paths.rb +++ b/railties/lib/rails/paths.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "pathname" + module Rails module Paths # This object is an extended hash that behaves as root of the Rails::Paths system. @@ -180,6 +182,14 @@ module Rails @paths end + def paths + raise "You need to set a path root" unless @root.path + + map do |p| + Pathname.new(@root.path).join(p) + end + end + def extensions # :nodoc: $1.split(",") if @glob =~ /\{([\S]+)\}/ end diff --git a/railties/test/application/paths_test.rb b/railties/test/application/paths_test.rb index 28a9206daa..3f1b188c81 100644 --- a/railties/test/application/paths_test.rb +++ b/railties/test/application/paths_test.rb @@ -51,6 +51,8 @@ module ApplicationTests assert_path @paths["config/locales"], "config/locales/en.yml" assert_path @paths["config/environment"], "config/environment.rb" assert_path @paths["config/environments"], "config/environments/development.rb" + assert_path @paths["config/routes.rb"], "config/routes.rb" + assert_path @paths["config/routes"], "config/routes" assert_equal root("app", "controllers"), @paths["app/controllers"].expanded.first end diff --git a/railties/test/application/routing_test.rb b/railties/test/application/routing_test.rb index bec038fb51..d4c67c2b03 100644 --- a/railties/test/application/routing_test.rb +++ b/railties/test/application/routing_test.rb @@ -265,6 +265,30 @@ module ApplicationTests assert_equal "WIN", last_response.body end + test "routes drawing from config/routes" do + app_file "config/routes.rb", <<-RUBY + AppTemplate::Application.routes.draw do + draw :external + end + RUBY + + app_file "config/routes/external.rb", <<-RUBY + get ':controller/:action' + RUBY + + controller :success, <<-RUBY + class SuccessController < ActionController::Base + def index + render plain: "success!" + end + end + RUBY + + app "development" + get "/success/index" + assert_equal "success!", last_response.body + end + { "development" => ["baz", "http://www.apple.com", "/dashboard"], "production" => ["bar", "http://www.microsoft.com", "/profile"] @@ -307,7 +331,7 @@ module ApplicationTests app_file "config/routes.rb", <<-RUBY Rails.application.routes.draw do - get 'foo', to: 'foo#bar' + draw :external get 'custom', to: 'foo#custom' get 'mapping', to: 'foo#mapping' @@ -316,6 +340,10 @@ module ApplicationTests end RUBY + app_file "config/routes/external.rb", <<-RUBY + get 'foo', to: 'foo#bar' + RUBY + app(mode) get "/foo" @@ -329,7 +357,7 @@ module ApplicationTests app_file "config/routes.rb", <<-RUBY Rails.application.routes.draw do - get 'foo', to: 'foo#baz' + draw :another_external get 'custom', to: 'foo#custom' get 'mapping', to: 'foo#mapping' @@ -338,6 +366,10 @@ module ApplicationTests end RUBY + app_file "config/routes/another_external.rb", <<-RUBY + get 'foo', to: 'foo#baz' + RUBY + sleep 0.1 get "/foo"