This pattern was introduced as a plugin by @dhh. The original implementation can be found in https://github.com/rails/routing_concerns
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -909,7 +909,7 @@ module Resources | ||
# CANONICAL_ACTIONS holds all actions that does not need a prefix or | ||
# a path appended since they fit properly in their scope level. | ||
VALID_ON_OPTIONS = [:new, :collection, :member] | ||
RESOURCE_OPTIONS = [:as, :controller, :path, :only, :except, :param] | ||
RESOURCE_OPTIONS = [:as, :controller, :path, :only, :except, :param, :concerns] | ||
CANONICAL_ACTIONS = %w(index create new show update destroy) | ||
|
||
class Resource #:nodoc: | ||
@@ -1046,6 +1046,8 @@ def resource(*resources, &block) | ||
resource_scope(:resource, SingletonResource.new(resources.pop, options)) do | ||
yield if block_given? | ||
|
||
concerns(options[:concerns]) if options[:concerns] | ||
|
||
collection do | ||
post :create | ||
end if parent_resource.actions.include?(:create) | ||
@@ -1210,6 +1212,8 @@ def resources(*resources, &block) | ||
resource_scope(:resources, Resource.new(resources.pop, options)) do | ||
yield if block_given? | ||
|
||
concerns(options[:concerns]) if options[:concerns] | ||
|
||
collection do | ||
get :index if parent_resource.actions.include?(:index) | ||
post :create if parent_resource.actions.include?(:create) | ||
@@ -1580,15 +1584,33 @@ def name_for_action(as, action) #:nodoc: | ||
end | ||
end | ||
|
||
module Concerns | ||
This comment has been minimized.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong.
carlosantoniodasilva
Member
|
||
def concern(name, &block) | ||
@concerns[name] = block | ||
end | ||
|
||
def concerns(*names) | ||
names.flatten.each do |name| | ||
if concern = @concerns[name] | ||
instance_eval(&concern) | ||
else | ||
raise ArgumentError, "No concern named #{name} was found!" | ||
end | ||
end | ||
end | ||
end | ||
|
||
def initialize(set) #:nodoc: | ||
@set = set | ||
@scope = { :path_names => @set.resources_path_names } | ||
@concerns = {} | ||
end | ||
|
||
include Base | ||
include HttpHelpers | ||
include Redirection | ||
include Scoping | ||
include Concerns | ||
include Resources | ||
end | ||
end | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
require 'abstract_unit' | ||
|
||
class CommentsController < ActionController::Base | ||
def index | ||
head :ok | ||
end | ||
end | ||
|
||
class ImageAttachmentsController < ActionController::Base | ||
def index | ||
head :ok | ||
end | ||
end | ||
|
||
class RoutingConcernsTest < ActionDispatch::IntegrationTest | ||
Routes = ActionDispatch::Routing::RouteSet.new.tap do |app| | ||
app.draw do | ||
concern :commentable do | ||
resources :comments | ||
end | ||
|
||
concern :image_attachable do | ||
resources :image_attachments, only: :index | ||
end | ||
|
||
resources :posts, concerns: [:commentable, :image_attachable] do | ||
resource :video, concerns: :commentable | ||
end | ||
|
||
resource :picture, concerns: :commentable do | ||
resources :posts, concerns: :commentable | ||
end | ||
|
||
scope "/videos" do | ||
concerns :commentable | ||
end | ||
end | ||
end | ||
|
||
include Routes.url_helpers | ||
def app; Routes end | ||
|
||
def test_accessing_concern_from_resources | ||
get "/posts/1/comments" | ||
assert_equal "200", @response.code | ||
assert_equal "/posts/1/comments", post_comments_path(post_id: 1) | ||
end | ||
|
||
def test_accessing_concern_from_resource | ||
get "/picture/comments" | ||
assert_equal "200", @response.code | ||
assert_equal "/picture/comments", picture_comments_path | ||
end | ||
|
||
def test_accessing_concern_from_nested_resource | ||
get "/posts/1/video/comments" | ||
assert_equal "200", @response.code | ||
assert_equal "/posts/1/video/comments", post_video_comments_path(post_id: 1) | ||
end | ||
|
||
def test_accessing_concern_from_nested_resources | ||
get "/picture/posts/1/comments" | ||
assert_equal "200", @response.code | ||
assert_equal "/picture/posts/1/comments", picture_post_comments_path(post_id: 1) | ||
end | ||
|
||
def test_accessing_concern_from_resources_with_more_than_one_concern | ||
get "/posts/1/image_attachments" | ||
assert_equal "200", @response.code | ||
assert_equal "/posts/1/image_attachments", post_image_attachments_path(post_id: 1) | ||
end | ||
|
||
def test_accessing_concern_from_resources_using_only_option | ||
get "/posts/1/image_attachment/1" | ||
assert_equal "404", @response.code | ||
end | ||
|
||
def test_accessing_concern_from_a_scope | ||
get "/videos/comments" | ||
assert_equal "200", @response.code | ||
end | ||
|
||
def test_with_an_invalid_concern_name | ||
e = assert_raise ArgumentError do | ||
ActionDispatch::Routing::RouteSet.new.tap do |app| | ||
app.draw do | ||
resources :posts, concerns: :foo | ||
end | ||
end | ||
end | ||
|
||
assert_equal "No concern named foo was found!", e.message | ||
end | ||
end |
91 comments
on commit 0dd2472
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhh I don't think, given the general tone of your response, that any alternative provided will receive serious consideration. That being said, I'm going to post one, anyway -- it doesn't really address the spirit of the assertions that @coreyhaines, @pixeltrix and others are making, but it does at least make some lemonade out of the given lemons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
David,
In keeping with your overall feeling about concerns' utility in the routes file, PR #7422 is a slight modification that at least provides for some code separation benefits as a result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general problem of this, with the ruby implementation looks very similar to memoizable
, which has been deprecated in favor of ruby's internal memoization.
If things like this can get removed because ruby allows to do the same very easily, why include concerns at all ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhh Feedback on why it fails? Could help refine it a bit more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reposing since the conversation is happening here:
@dhh At a minimum, it allows us to shorten the routes file considerably by moving concerns out into their own separate files (not shown here).
That would seem to pass the basic before/after test you're proposing. leading to a header in the routes file that read like:
concerns :commentable, Commentable
concerns :reviewable, ReviewableConcern.new(some: 'initializaton params')
With a minor modification, it could also eliminate the need for the second parameter altogether, via the same classifying logic as is used elsewhere.
Beyond this, there is strong preference in the Ruby community for implementations that allow duck-typed objects to be swapped in where appropriate.
Consider the differences between CarrierWave and Paperclip, for instance. The former allows for much more idiomatic separation of code, reducing the amount of noise we have to read through in order to parse the basics, and points us to a specific location to find out more should we so choose.
It's less about simplifying the writing of the code (though some would argue that anything that allows us to break things into a separate object if we so want will be a win) and more about a further "improvement" to readability, if we're going to go down this route. (pun not intended, but awesome)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reposted the reply as well:
But if you look at the actual use cases this was extracted from, they don't justify an external class. They're too small. They're usually just a single line or two. Moving them into their own classes and putting those classes outside of the routing.rb file doesn't clarify things in my mind.
This has similarities to the "actions should be classes" debate. I don't think they should. There's not enough there to justify it and it makes things harder to follow.
Again, though, I very much appreciate this level of debate, Ernie. It's focused around real code alternatives, so it's concrete. Not abstract, hand-wavy "I've seen teams use this wrong!" kind of arguments. Thank you for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't particularly care about the 'accepts a class' bit, but Validations now work this way: custom class, symbol that references a method name, or a block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhh Maybe -- my gut feeling is that since this doesn't break the previous style of usage, at all, it's a net positive.
I don't have time to write up additional code examples, but I'm thinking that developers of engines may find it useful.
At the very least, it allows us to intelligently select the controller (and therefore, associated view files, and on down the line) that handles the comment functionality, without having to use a separate concern, reducing the working vocabulary of the application reflected in the routes file to only the level of detail necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And being able to intelligently select an appropriate controller for a specific functionality would reduce the likelihood of developers creating ridiculous filter-soup in their controllers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also important to note that by accepting a callable, we aren't limited to only classes, but any object responding to call, generated by any means we deem appropriate. I used a class that had a class method in the test only because that was the most expedient way to show the functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coreyhaines "I have seen teams misusing features" is not an argument against adding features in my view. Sometimes people misuse stuff, sometimes people just do not know what does Rails offer. That is normal, and it is the consultant's job to help teams tune their Rails skills.
New features have to be discussed by their own merits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corey's argument is not "I've seen teams mis-use this feature" it's that "A team that uses this feature ends up regretting it.":
As I sit with teams, I don't find they've "abused" the features, they simply used them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@steveklabnik - Either way, what @fxn stated still stands: Sometimes people mis-use stuff. Isn't that the responsibility of the developer using the framework, to understand what the framework offers and then make the best decisions - architecturally or otherwise?
I can bet that even if it was designed as @coreyhaines would prefer that people would still mis-use it. There is no silver bullet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but since we're not talking about people mis-using a feature, then it's irrelevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reading all of this, I fail to see an argument as to why this needs to be in rails core instead of a gem? Is it "I say so" argument or is there a valid argument that this needs to be in rails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@steveklabnik Corey mentions misuses of AS::Concern as an example to depict what he means. It is not just that people use the features of Rails, of course people use them! Or do you build Rails applications to write CGI scripts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@steveklabnik - the regret of teams using a feature is because they mis-used it in the first place. I have examples of this, too. It is relevant according to @coreyhaines. The documentation/guides describe how the feature works - not prescribe how a team or individual should use it. That's up to the individual or team to do their own research. Rails can't continue to be built with safety nets everywhere because people might mis-use it and then later regret it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nateklaiber What Corey proposes however is not doing anything and leaving things as-is, since you could use method or lambdas right now for this.
New features have to be discussed by their own merits.
@fnx Agreed.
In this particular case, however, I don't see how this adds anything more than (slight) bloat to the framework and a DSL for the sake of having a DSL. This adds complexity, even if it isn't much compared to the framework as a whole. It means newcomers have to learn "one more thing", and adds more code to maintain and debug.
That said, I'm not the one calling the shots here, and it's clear that no matter how many arguments people come up with against the feature, the feature will stay. So let's just move on and do productive stuff :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nateklaiber I think there's some room for a distinction to be made between "features" and "tasty treats that happen to be sitting on a mousetrap." I'm pretty sure @coreyhaines was talking about the latter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously, I'm sitting on both sides of the fence, here. I'm pretty much resigned to seeing this stay in core (though I'd prefer it remain a gem), but as such, I'd sure like to see something (#7422) that would allow for slightly improved separation of concerns (hah!).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case @dhh had a pain and he solved it in BCX using the given technique . While others are thinking it will cause more problems and they might be right. However they have not used this new feature while @ddh put it to use and he liked it.
So as Jason will say "give it 5 minutes" http://37signals.com/svn/posts/3124-give-it-five-minutes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@foca - Isn't that a slippery slope, though? Why use Rails at all then - if you could just use Ruby method or lambdas? In this case, it seems to come down to taste. You could argue that things added from Rails 1.X until now have been adding slight bloat and complexity. It's an evolution of the framework.
@ernie - I agree that I am both sides of the fence, too. Personally, I am more leaning towards it being a gem.
Can you protect developers from "tasty treats that happen to be sitting on a mousetrap" - that is an individual developer thing. It's a level of professionalism and knowledge. People can mess up plain Ruby, too, based on their mis-understandings. That argument just doesn't sit well with me as it's not something a framework or language will ever solve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@neerajdotname I don't think anyone is arguing that @dhh didn't solve a problem by using this. It's more of a discussion as to why this needs to be in rails-core.
Unless I missed it, I haven't see a real reason as to why it needs to be in core other than "I said so".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@revans I think @dhh nicely described why it needs to be in core. The main argument is that rails is a collection of best practices. If a particular way of doing thing is on the fringe and it should not be mainstream then it should not be in core. Case in point is ActiveResource
. It used to be core but now it is a plugin.
On the flip side strong_parameters
started as a plugin but it is a better solution than attr_accessible
so attr_accessible
is moving to plugin and strong_parameters
is coming from plugin to core.
Same argument can be made about asset pipeline
. When it landed there was a strong voice that it should be left as plugin. However there were enough good things in it to bring it to core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should give credit to rails core team for not only bringing new features to core but also for retiring features that do not justify to be in core. Many things like ActiveResource
, RJS
, rails api
, attr_accessible
have been removed from core. Yes rails api
code was merged to master and it was removed before the release.
So just because something is in master do not think it will never go out of core. As I said ( or as Jason said ) give it 5 minutes and if it is not worthy its weight it will go out.
And just like coffeescript debate you do not have to use it. Rails is a collection of features. Use the features that you like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhh I understand that. I've been a long time user of Rails and have enjoyed those benefits.
Other than your use case, I am not aware (admittedly, I haven't gone looking either) of others. So for the reason as to why this needs to be in core, hasn't been apparent to me. If it's as simple as a pattern you like, I get it. Not sure I agree it needs to be in core, but it's your baby.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The concepts of code reuse, DRY-ness, and describing such succinctly and clearly, are all good ones. This change encourages everyone to do more of these, and to do it better. So adding concerns to the router is a good thing.
Sure people have always been able to do it in slightly longer more verbose ways, and they still can if they choose. I'm speaking of defining methods here, if you have a large app with a lot of modular pieces, those "few lines" longer can really add up. I for one will be switching to concerns because then I know I'm safe in not stepping on any previously-defined methods any longer (I'm losing enough hair as it is without debugging that)...
Also the way things can nest now has encouraged more code reuse in rails 3, compared to rails 2. This concerns thing is just more evolution of the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see why @dhh wants this in the routing DSL, managing a routes file using purely DSL is just more consistent and pleasant. Sure you can use Ruby code to manage your routes, but to me it just doesn't feel as the way it was intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @revans, shouldn't it be a gem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defining methods inside the draw DSL is a visual wart that doesn't fit with the rest of the flow.
That's true. By using 1.9 lambda syntax, though, it could start to look nicer perhaps. E.g.: commentable = ->{ resources :comments }
.. and you could then support both methods of inclusion, whether through a concerns: commentable
argument or with the @pixeltrix approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed a flaw in the method alternative that was suggested initially. Calling a method defined in the top of a route set will generate resources from the context the method was defined in -- in other words, it will not know about its current mapper when adding resources, or any scoping.
Also fixed a flaw in my alternative implementation that suffered from a similar issue -- it wasn't caught in the existing tests, which weren't checking for context.
With #7422, we'd have the option of doing:
concern :commentable do
resources :comments
end
def commentable(mapper)
mapper.resources :comments
end
concern :commentable, method(:commentable)
or, for something that just shouldn't be in the routes file:
# Separate file
class PurchasableConcern
RETURNABLES = Departments.map(&:name) - %w(pets electronics)
def self.call(mapper)
mapper.resources :purchases
mapper.resources :receipts
mapper.resources :returns if RETURNABLES.include?(mapper.current_scope[:controller])
end
end
# routes.rb
concern :purchasable, PurchasableConcern
We gain the ability to behave differently based on the scope we're in using all fo these options, but I wouldn't think it would make sense except in the last case, since the idea would be to make the routes more readable by saying that something was something-able, and not delve into the specific implementation unless someone wanted to go looking for it.
Disclaimer: none of this should be construed to indicate that I think this is a sound idea at the fundamental level, but if we're gonna do it, then we should try to at least get some code separation options out of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resources :toys do
concern :purchasable, PurchasableConcern.new(include_returns: true)
end
resources :snacks do
concern :purchasable, PurchasableConcern.new(include_returns: false)
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yfeldblum concerns would be defined outside the resource that uses them, then reused, so not quite like your example shows. That being said, it'd be trivial to allow concerns to accept options, and I like the idea of passing in options (telling) instead of the callable asking about the current scope of the mapper. Callables really shouldn't be asking about the mapper's scope as much as reacting to options like returnable: true
. Updating PR to reflect that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, updated #7422 accordingly. Added better documentation, as well. I think the documentation outlines the use case pretty well, so including here:
concern - Define a routing concern using a name.
Concerns may be defined inline, using a block, or handled by
another object, by passing that object as the second parameter.
The concern object, if supplied, should respond to call
,
which will receive two parameters:
- The current mapper
- A hash of options which the concern object may use
Options may also be used by concerns defined in a block by accepting
a block parameter. So, using a block, you might do something as
simple as limit the actions available on certain resources, passing
standard resource options through the concern:
concern :commentable do |options|
resources :comments, options
end
resources :posts, concerns: :commentable
resources :archived_posts do
# Don't allow comments on archived posts
concerns :commentable, only: [:index, :show]
end
Or, using a callable object, you might implement something more
specific to your application, which would be out of place in your
routes file.
# purchasable.rb
class Purchasable
def initialize(defaults = {})
@defaults = defaults
end
def call(mapper, options = {})
options = @defaults.merge(options)
mapper.resources :purchases
mapper.resources :receipts
mapper.resources :returns if options[:returnable]
end
end
# routes.rb
concern :purchasable, Purchasable.new(returnable: true)
resources :toys, concerns: :purchasable
resources :electronics, concerns: :purchasable
resources :pets do
concerns :purchasable, returnable: false
end
Any routing helpers can be used inside a concern. If using a
callable, they're accessible from the Mapper that's passed to
call
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally (and @josevalim would be able to better confirm/deny this than me) it seems as a change like this would enable much of Devise's mapping code (https://github.com/plataformatec/devise/blob/master/lib/devise/mapping.rb) to be handled by an Authenticatable concern against a users resource.
If not directly applicable to Devise, it would certainly be a worthwhile hook for other authentication implementations.
To me:
concern :authenticatable, MyAuthenticationThing.new(some: 'opts')
resources :users
concerns :authenticatable, authentication_methods: [:token, :password]
end
would be preferable to using macros to deliver similar functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only... worry... about @ernie's implementation is the location of the concern file. I don't think that dumping it in the lib
would be a great idea, much like dumping anything in lib
is not a great idea.
What would the convention be for these concern files? lib/concerns/purchasable.rb
? config/routing/purchasable.rb
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think config/routing or config/routes makes the most sense as a home for routing concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest moving routes to app/routes
if possible. After all, they aren't really config: they are actually the top-level interface to your application, mapping HTTP requests to the classes and methods that handle these requests.
There is also certainly a need in larger applications to split up routes files in general, because they can grow to hundreds of lines or more with multiple groups of routes that are internally related but which do not relate strongly to each other. This need applies to the concept of routing concerns, but it's also a general need even for routes files which don't have any concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest moving routes to app/routes if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhh In the case of BCX, it seems like the routing there is more amenable to concerns. But there are other applications where there is just a lot of routing and not very much commonality at all (and where attempting to impose commonality is dumb). Redmine's config/routes.rb, for example, has about 300 lines of routing code. I think a good DSL for that case would hit the spot for those large applications with lots of routing but not much commonality.
MyApp::Application.routes.draw do
# includes routes from app/routes/storefront_routes.rb which has 200 lines
draw_routes "storefront"
# includes routes from app/routes/admin_routes.rb which has 150 lines
draw_routes "admin"
# includes routes from app/routes/helpdesk_routes.rb which has 50 lines
draw_routes "helpdesk"
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhh OK, here's a situation, extracted from a slightly-modified case in an old app.
When you're doing things that involve social interactions, you end up with tons of comments, abuse flags, subscriptions, likes, and so on. A bunch of these are actually more or less toggleable on/off switches for users, and could probably inherit from the same controller. A preference of mine is to avoid trying to iterate through params looking for a likely suspect when it comes to a polymorphic relationship's nested resources, and instead specify the base, directly, so that I can find the "-able" without putting undue trust in user-submitted params.
This leads to a pattern like:
Some::Application.routes.draw do
resources :posts do
resources :likes, base: 'post'
resources :flags, base: 'post'
resources :subscriptions, base: 'post'
resources :comments, base: 'post' do
resources :likes, base: 'comment'
resources :flags, base: 'comment'
end
end
resources :photos do
resources :likes, base: 'photo'
resources :flags, base: 'photo'
resources :comments, base: 'photo' do
resources :likes, base: 'comment'
resources :flags, base: 'comment'
end
end
end
Which only gets more unmanageable over time. With my patch, this is the after:
class SociallyInteractable
def initialize(*resource_list)
@resource_list = resource_list
end
def call(mapper, options)
filtered_resources(options).each do |res|
mapper.instance_eval do
resources res, base: @scope[:controller].singularize do
if res == :comments
concerns :socially_interactable, only: [:likes, :flags]
end
end
end
end
end
def filtered_resources(options)
if options[:except]
@resource_list - Array(options[:except])
elsif options[:only]
@resource_list & Array(options[:only])
else
@resource_list
end
end
end
Some::Application.routes.draw do
concern :socially_interactable,
SociallyInteractable.new(:likes, :flags, :subscriptions, :comments)
resources :posts do
concerns :socially_interactable
end
resources :photos do
concerns :socially_interactable, except: :subscriptions
end
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's reasonably compelling. Thank you.
Anyone else have some similar use cases they could post based on this? I'm close to convinced that this is +1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, @ernie, I'm curious as to what insight lead you to switch from "This isn't even syntactic sugar. It's more like syntactic aspartame. It might slim down your code, but it's also a carcinogen" to extending and improving the feature. It seems like the SociallyInteractable extraction you just based your extension off does indeed materially cut down on the complexity of the sample routes.rb file. I take it that you see this as a good thing, ye?
(I'm not being I'm-told-you-so, I'm genuinely interested in learning what argument or insight made you change your position. We have more stuff coming in the vain of concerns for Rails 4, so I'd like to present those changes with the most effective and persuasive arguments.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhh I'm of the strong opinion that these types of changes should do what they can to:
- Allow for idioms that have become second-nature to Rubyists (in this case, duck-typing with #call, in a similar vein as Rack)
- Enable and encourage developers to extract reusable code into easily-testable components. Writing a unit test for the logic being used in SociallyInteractable, above, is simple. The fact that my first implementation (and the method suggestions in this thread) were actually fundamentally broken when it came to nested resources but were not caught by the test suite nor any of those giving them +1s is an indication, to me, that this stuff can be tricky to test, without simply testing each and every path in a routes file. Better to be able to test the logic used to generate the paths, if possible.
- Related to the first two, really. Where possible, I'd love to see us continue the trend toward modularity we saw in Rails 3, with Rails 4. One way to continue to move in this direction is to adhere to SOLID principles as we build features. I believe that my patch moves us further in that direction, defining a clear API that one must adhere to if one wants to behave as a concern, and then allowing objects to be substituted, Liskov-style, according to that API.
As I see it, the previous implementation did not address those goals, and for no good reason, as far as I could tell.
I can't speak for @coreyhaines, and I know he has since checked out of this thread, but I think that would probably summarize at least part of his position, no doubt less eloquently as he would, himself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhh I didn't really think I was spewing venom, for my part, but I was passionate. Clearly, my words were offensive, so I apologize -- particularly if that offense has led to a steeper-than-necessary climb to merge.
My comment was directly related to a concern about the previous implementation, which seemed like the addition of indirection using a less idiomatic means that would be trickier to understand, at first glance. I wouldn't have known that the concern was a declaration, since the convention of other blocks within the routes.rb DSL is to define routes on the spot.
In the previous implementation, the reader is unlikely to know what it is that they are seeing, at first glance. Is the concern a top-level entity like a mapping? Is it a declaration of something else? When does the code get invoked?
My PR was an attempt to partially right that perceived (correctly or incorrectly) wrong, by allowing us to use a syntax when declaring a concern that is more obviously a statement that we are assigning a name to a reusable component.
I think (hope) that the adjustments I am suggesting would make these things more clear, to more people.
Why is this a module? I see it being included a single place and it clearly is heavily dependent on being included into this exact class because it has a data dependency on @concerns. I'd imagine it's not designed to be used anywhere else, so why bother?