Skip to content

Commit

Permalink
Revert "Allow loading external route files from the router"
Browse files Browse the repository at this point in the history 
This reverts commit 6acebb3.

Usage of this feature did not reveal any improvement in existing apps.

Conflicts:

	actionpack/lib/action_dispatch/routing/mapper.rb
	guides/source/routing.textile
	railties/lib/rails/engine.rb
	railties/lib/rails/paths.rb
	railties/test/paths_test.rb
  • Loading branch information
@josevalim
josevalim committed Jun 29, 2012
1 parent 50b6110 commit 5e7d6bb
Show file tree
Hide file tree
Showing 10 changed files with 6 additions and 149 deletions.
17 changes: 0 additions & 17 deletions actionpack/lib/action_dispatch/routing/mapper.rb
View file
Expand Up @@ -1316,22 +1316,6 @@ def shallow?
parent_resource.instance_of?(Resource) && @scope[:shallow]
end

def draw(name)
path = @draw_paths.find do |_path|
File.exists? "#{_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

# match 'path' => 'controller#action'
# match 'path', to: 'controller#action'
# match 'path', 'otherpath', on: :member, via: :get
Expand Down Expand Up @@ -1581,7 +1565,6 @@ def name_for_action(as, action) #:nodoc:

def initialize(set) #:nodoc:
@set = set
@draw_paths = set.draw_paths
@scope = { :path_names => @set.resources_path_names }
end

Expand Down
2 changes: 0 additions & 2 deletions actionpack/lib/action_dispatch/routing/route_set.rb
View file
Expand Up @@ -237,7 +237,6 @@ def optimized_helper(route)
attr_accessor :formatter, :set, :named_routes, :default_scope, :router
attr_accessor :disable_clear_and_finalize, :resources_path_names
attr_accessor :default_url_options, :request_class, :valid_conditions
attr_accessor :draw_paths

alias :routes :set

Expand All @@ -249,7 +248,6 @@ def initialize(request_class = ActionDispatch::Request)
self.named_routes = NamedRouteCollection.new
self.resources_path_names = self.class.default_resources_path_names.dup
self.default_url_options = {}
self.draw_paths = []

self.request_class = request_class
@valid_conditions = { :controller => true, :action => true }
Expand Down
18 changes: 0 additions & 18 deletions guides/source/routing.textile
View file
Expand Up @@ -845,24 +845,6 @@ end

This will create routing helpers such as +magazine_periodical_ads_url+ and +edit_magazine_periodical_ad_path+.

h3. Breaking Up a Large Route File

If you have a large route file that you would like to break up into multiple files, you can use the +#draw+ method in your router:

<ruby>
draw :admin
</ruby>

Then, create a file called +config/routes/admin.rb+. Name the file the same as the symbol passed to the +draw+ method. You can then use the normal routing DSL inside that file:

<ruby>
# in config/routes/admin.rb

namespace :admin do
resources :posts
end
</ruby>

h3. Inspecting and Testing Routes

Rails offers facilities for inspecting and testing your routes.
Expand Down
13 changes: 4 additions & 9 deletions railties/lib/rails/application/routes_reloader.rb
View file
Expand Up @@ -3,13 +3,12 @@
module Rails
class Application
class RoutesReloader
attr_reader :route_sets, :paths, :external_routes
attr_reader :route_sets, :paths
delegate :execute_if_updated, :execute, :updated?, :to => :updater

def initialize
@paths = []
@route_sets = []
@external_routes = []
@paths = []
@route_sets = []
end

def reload!
Expand All @@ -24,11 +23,7 @@ def reload!

def updater
@updater ||= begin
dirs = @external_routes.each_with_object({}) do |dir, hash|
hash[dir.to_s] = %w(rb)
end

updater = ActiveSupport::FileUpdateChecker.new(paths, dirs) { reload! }
updater = ActiveSupport::FileUpdateChecker.new(paths) { reload! }
updater.execute
updater
end
Expand Down
7 changes: 1 addition & 6 deletions railties/lib/rails/engine.rb
View file
Expand Up @@ -508,10 +508,7 @@ def env_config
# Defines the routes for this engine. If a block is given to
# routes, it is appended to the engine.
def routes
@routes ||= ActionDispatch::Routing::RouteSet.new.tap do |routes|
routes.draw_paths.concat paths["config/routes"].paths
end

@routes ||= ActionDispatch::Routing::RouteSet.new
@routes.append(&Proc.new) if block_given?
@routes
end
Expand Down Expand Up @@ -555,12 +552,10 @@ def load_seed

initializer :add_routing_paths do |app|
paths = self.paths["config/routes.rb"].existent
external_paths = self.paths["config/routes"].paths

if routes? || paths.any?
app.routes_reloader.paths.unshift(*paths)
app.routes_reloader.route_sets << routes
app.routes_reloader.external_routes.unshift(*external_paths)
end
end

Expand Down
1 change: 0 additions & 1 deletion railties/lib/rails/engine/configuration.rb
View file
Expand Up @@ -53,7 +53,6 @@ def paths
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"
paths.add "db/seeds.rb"
Expand Down
10 changes: 1 addition & 9 deletions railties/lib/rails/paths.rb
View file
Expand Up @@ -114,7 +114,7 @@ def filter_by(constraint)
class Path
include Enumerable

attr_reader :path, :root
attr_reader :path
attr_accessor :glob

def initialize(root, current, paths, options = {})
Expand Down Expand Up @@ -180,14 +180,6 @@ def to_ary
@paths
end

def paths
raise "You need to set a path root" unless @root.path

map do |p|
File.join @root.path, p
end
end

# Expands all paths against the root and return all unique values.
def expanded
raise "You need to set a path root" unless @root.path
Expand Down
2 changes: 0 additions & 2 deletions railties/test/application/paths_test.rb
View file
Expand Up @@ -50,8 +50,6 @@ def assert_not_in_load_path(*path)
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
Expand Down
83 changes: 0 additions & 83 deletions railties/test/application/routing_test.rb
View file
Expand Up @@ -178,90 +178,7 @@ def index
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 :text => "success!"
end
end
RUBY

app 'development'
get '/success/index'
assert_equal 'success!', last_response.body
end

{"development" => "baz", "production" => "bar"}.each do |mode, expected|
test "reloads routes when external configuration is changed in #{mode}" do
controller :foo, <<-RUBY
class FooController < ApplicationController
def bar
render :text => "bar"
end
def baz
render :text => "baz"
end
end
RUBY

app_file 'config/routes.rb', <<-RUBY
AppTemplate::Application.routes.draw do
draw :external
end
RUBY

app_file 'config/routes/external.rb', <<-RUBY
get 'foo', :to => 'foo#bar'
RUBY

app(mode)

get '/foo'
assert_equal 'bar', last_response.body

app_file 'config/routes/external.rb', <<-RUBY
get 'foo', :to => 'foo#baz'
RUBY

sleep 0.1

get '/foo'
assert_equal expected, last_response.body

app_file 'config/routes.rb', <<-RUBY
AppTemplate::Application.routes.draw do
draw :external
draw :other_external
end
RUBY

app_file 'config/routes/other_external.rb', <<-RUBY
get 'win', :to => 'foo#baz'
RUBY

sleep 0.1

get '/win'

if mode == "development"
assert_equal expected, last_response.body
else
assert_equal 404, last_response.status
end
end

test "reloads routes when configuration is changed in #{mode}" do
controller :foo, <<-RUBY
class FooController < ApplicationController
Expand Down
2 changes: 0 additions & 2 deletions railties/test/paths_test.rb
View file
Expand Up @@ -29,7 +29,6 @@ def setup
test "creating a root level path" do
@root.add "app"
assert_equal ["/foo/bar/app"], @root["app"].to_a
assert_equal ["/foo/bar/app"], @root["app"].paths
end

test "creating a root level path with options" do
Expand Down Expand Up @@ -192,7 +191,6 @@ def setup
@root["app"] = "/app"
@root["app"].glob = "*.rb"
assert_equal "*.rb", @root["app"].glob
assert_equal ["/foo/bar/app"], @root["app"].paths
end

test "it should be possible to override a path's default glob without assignment" do
Expand Down

20 comments on commit 5e7d6bb

@henrik
Copy link
Contributor

@henrik henrik commented on 5e7d6bb Jul 2, 2012

Choose a reason for hiding this comment

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

@josevalim What do you mean by "Usage of this feature did not reveal any improvement in existing apps."? Compared to doing it manually, without special framework support?

I've found it useful to split routes into multiple files on larger projects, but IIRC it was easy enough with just multiple My::Application.routes.draw or instance_eval or something.

@rafaelfranca
Copy link
Member

Choose a reason for hiding this comment

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

This was reverted in favor of https://github.com/rails/routing_concerns, that we think is a better approach.

@henrik
Copy link
Contributor

@henrik henrik commented on 5e7d6bb Jul 2, 2012

Choose a reason for hiding this comment

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

@rafaelfranca Thanks! I don't see from the README how routing_concerns does anything to help split up routes, though – how would you go about that?

@steveklabnik
Copy link
Member

Choose a reason for hiding this comment

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

The point of 'splitting up routes' was to make the routes file easier to read. routing_concerns will (hopefully) DRY up your routes so that you don't need to split them up anymore.

@henrik
Copy link
Contributor

@henrik henrik commented on 5e7d6bb Jul 2, 2012

Choose a reason for hiding this comment

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

@steveklabnik Alright, thanks!

@kuraga
Copy link

@kuraga kuraga commented on 5e7d6bb Jul 3, 2012

Choose a reason for hiding this comment

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

But Rails makes possible to split much: locales, migrations... It's useful for spliting-up-by-components very much!

@kuraga
Copy link

@kuraga kuraga commented on 5e7d6bb Jul 10, 2012

Choose a reason for hiding this comment

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

@steveklabnik amn't I right (see previous message)?

@steveklabnik
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if you're right or not. Personally, I never tried this feature. @dhh didn't find it very helpful.

@atrauzzi
Copy link

Choose a reason for hiding this comment

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

Are there any examples of this new approach to defining multiple routes? Rails 4 docs make it look like concerns can only be used to modify existing routes...?

@robin850
Copy link
Member

Choose a reason for hiding this comment

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

Rails 4 docs make it look like concerns can only be used to modify existing routes...?

Actually concern routes are provided for DRY concerns, this is not really an alternative to the reverted patch here. I guess there is no equivalent at the time being but I may be wrong. It looks like this is not so hard to implement such feature inside your application.

@dhh
Copy link
Member

@dhh dhh commented on 5e7d6bb Mar 30, 2014 via email

Choose a reason for hiding this comment

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

@atrauzzi
Copy link

Choose a reason for hiding this comment

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

Agreed, especially if the language already supports it, no need to try and over-systemize it. I think I've got something figured out.

@ryana
Copy link
Contributor

@ryana ryana commented on 5e7d6bb Oct 9, 2015

Choose a reason for hiding this comment

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

I find this really useful, so I followed the gist linked to here to get the basic functionality working.

The problem I'm running into now is the lack of autoloader support. I see in this commit that a bunch of integration points had to be made to autoload the files in config/routes/*.rb. Does Rails 4 have some cool API I don't know about that will allow me to affect the same changes without monkeypatching?

Thanks!

@codextremist
Copy link

Choose a reason for hiding this comment

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

Splitting a large routes.rb file in smaller parts can be very helpful, specially if you have totally independent routes. A classical example would be an app with many different user types (admin, members, guests, etc). Concerns could even be shared between all of them. Anyways, concerns helps a lot on code reusability, but not necessarily on code readability. Anyway the need for native support depends on the eye of the beholder

@h0jeZvgoxFepBQ2C
Copy link

@h0jeZvgoxFepBQ2C h0jeZvgoxFepBQ2C commented on 5e7d6bb Dec 7, 2016 •
edited

Choose a reason for hiding this comment

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

From my point of view you removed a really great feature.. Concerns are nice, but if you really want to seperate big route sets (like for components: apis, backend, frontend1, frontend2) you are now lost. I've tried the instance_eval solution, but this doesn't reload my routes in development..

I would be really happy if you could bring back this functionality.

Edit:

I'm now going with this approach, but it's still not reloading in my dev environment if I change something (see http://stackoverflow.com/a/34798146/74264):

# config/application.rb
config.autoload_paths << Rails.root.join('config/routes')

# config/routes.rb
Rails.application.routes.draw do
  root to: 'home#index'

  extend ApiRoutes
end

# config/routes/api_routes.rb
module ApiRoutes
  def self.extended(router)
    router.instance_exec do
      namespace :api do
        resources :tasks, only: [:index, :show]
      end
    end
  end
end

@tomaadland
Copy link

Choose a reason for hiding this comment

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

@lichtamberg did you try the answer below ? This is how its been done in Gitlab (see https://gitlab.com/gitlab-org/gitlab-ce/blob/master/config/initializers/routing_draw.rb) works like charm :)

@elegantcoder
Copy link

@elegantcoder elegantcoder commented on 5e7d6bb Jun 6, 2017 •
edited

Choose a reason for hiding this comment

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

I had reload issue similar to @lichtamberg .
And I re-implemented Rails::Application#routes_reloader to solve this issue.

It works nice so far.

module YourProject
  class Application < Rails::Application
  
  def routes_reloader
        @routes_reloader ||= RoutesReloader.new.tap do |routes_reloader|
          routes_reloader.paths.push(*Pathname.glob(Rails.root.join('config', 'routes') + '*.rb'))
        end
      end
  end
end

@rainerborene
Copy link

Choose a reason for hiding this comment

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

with the new direct directive on routes.rb this feature definitely should be back. for now I will be using the GitLab solution described by @tomaadland.

@h0jeZvgoxFepBQ2C
Copy link

Choose a reason for hiding this comment

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

@tomaadland I know some time went by, but right now I have this issue up again. Is your mentioned solution from gitlab not very imperformant? Loading the route files every request looks not very optimal for me.

Right now I couldn't find any solution to make it work with routing concerns, so I really miss the built-in feature to split up routes.

@rafaelfranca
Copy link
Member

Choose a reason for hiding this comment

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

This feature exists on Rails master already #37892

Please sign in to comment.