New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow multiple `root` routes in same scope level #20940

Merged
merged 1 commit into from Oct 10, 2015

Conversation

Projects
None yet
4 participants
@rafaelsales
Contributor

rafaelsales commented Jul 19, 2015

TL;DR:
Currently I have:

get '/', to: 'portfolio#show', constraints: ->(req) { Hostname.portfolio_site?(req.host) }
get '/', to: 'blog#show',      constraints: ->(req) { Hostname.blog_site?(req.host) }
root 'landing#show'

But I would like to have:

root 'portfolio#show', constraints: ->(req) { Hostname.portfolio_site?(req.host) }
root 'blog#show',      constraints: ->(req) { Hostname.blog_site?(req.host) }
root 'landing#show'

Background: When an application has multiple root entries in same scope with different constraints, the current solution is to use get '/' instead of simply root. This is because when we try to add another entry of root, Rails complains there's already a route with root name.
The same doesn't happen with multiple get's (or other matchers). So, I think it's fair that root also allows it since it's just a shortcut for a get internally.

PS: I couldn't find a spec related to allowing multiple get's, post's, etc with same path. If you know where is it, I could implement a similar spec for root.

@seuros seuros added the actionpack label Jul 19, 2015

@rafaelsales

This comment has been minimized.

Contributor

rafaelsales commented Jul 22, 2015

Anything needed from my end to have feedback here?

@rafaelsales rafaelsales force-pushed the rafaelsales:allow-multiple-root-routes branch from aaa6b83 to 524551d Jul 22, 2015

@seuros

This comment has been minimized.

Member

seuros commented Jul 22, 2015

LGTM
cc @rafaelfranca

@rafaelsales rafaelsales force-pushed the rafaelsales:allow-multiple-root-routes branch from 524551d to ba838ff Jul 22, 2015

@rafaelsales

This comment has been minimized.

Contributor

rafaelsales commented Jul 24, 2015

@arthurnn

This comment has been minimized.

Member

arthurnn commented Oct 5, 2015

Not sure about this honestly. the multiple get '/', look good to me already. I think this will add a few more bits of complexity to the code, that can already handle the use case described.
👎 from me.

@rafaelsales

This comment has been minimized.

Contributor

rafaelsales commented Oct 5, 2015

But then routes DSL is inconsistent. You can have multiple root but with a "hack", using get '/'
Also, where this adds complexity to code? Instead, this makes the code consistent.
There are other inconsistencies in routes DSL, but I figured rails codebase looks like a ghost city

@rafaelfranca rafaelfranca self-assigned this Oct 8, 2015

@@ -586,9 +587,8 @@ def with_default_scope(scope, &block)
end
end
# Query if the following named route was already defined.

This comment has been minimized.

@rafaelfranca

rafaelfranca Oct 8, 2015

Member

Why the documentation was removed?

@rafaelsales rafaelsales force-pushed the rafaelsales:allow-multiple-root-routes branch 3 times, most recently Oct 10, 2015

Allow multiple `root` routes in same scope level
When an application has multiple root entries with different
constraints, the current solution is to use `get '/'`. Example:

**Currently I have to do:**
```ruby
get '/', to: 'portfolio#show', constraints: ->(req) { Hostname.portfolio_site?(req.host) }
get '/', to: 'blog#show',      constraints: ->(req) { Hostname.blog_site?(req.host) }
root 'landing#show'
```

**But I would like to do:**
```ruby
root 'portfolio#show', constraints: ->(req) { Hostname.portfolio_site?(req.host) }
root 'blog#show',      constraints: ->(req) { Hostname.blog_site?(req.host) }
root 'landing#show'
```

Other URL matchers such as `get`, `post`, etc, already allows this, so I
think it's fair that `root` also allow it since it's just a shortcut for
a `get` internally.

@rafaelsales rafaelsales force-pushed the rafaelsales:allow-multiple-root-routes branch to 4db921a Oct 10, 2015

@rafaelsales

This comment has been minimized.

Contributor

rafaelsales commented Oct 10, 2015

@rafaelfranca Removed that doc by mistake. Fixed and rebased

rafaelfranca added a commit that referenced this pull request Oct 10, 2015

Merge pull request #20940 from rafaelsales/allow-multiple-root-routes
Allow multiple `root` routes in same scope level

@rafaelfranca rafaelfranca merged commit c61826e into rails:master Oct 10, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rafaelsales

This comment has been minimized.

Contributor

rafaelsales commented Oct 10, 2015

ty

pixeltrix added a commit that referenced this pull request Aug 29, 2016

Fix nested multiple roots
The PR #20940 enabled the use of multiple roots with different constraints
at the top level but unfortunately didn't work when those roots were inside
a namespace and also broke the use of root inside a namespace after a top
level root was defined because the check for the existence of the named route
used the global :root name and not the namespaced name.

This is fixed by using the name_for_action method to expand the :root name to
the full namespaced name. We can pass nil for the second argument as we're not
dealing with resource definitions so don't need to handle the cases for edit
and new routes.

Fixes #26148.

pixeltrix added a commit that referenced this pull request Aug 29, 2016

Fix nested multiple roots
The PR #20940 enabled the use of multiple roots with different constraints
at the top level but unfortunately didn't work when those roots were inside
a namespace and also broke the use of root inside a namespace after a top
level root was defined because the check for the existence of the named route
used the global :root name and not the namespaced name.

This is fixed by using the name_for_action method to expand the :root name to
the full namespaced name. We can pass nil for the second argument as we're not
dealing with resource definitions so don't need to handle the cases for edit
and new routes.

Fixes #26148.

(cherry picked from commit 2ea66fc)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment