Skip to content

Bring back feature that allows loading external route files: #37892

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

Conversation

Edouard-chin
Copy link
Member

Bring back feature that allows loading external route files:

This feature existed back in 2012
5e7d6bb
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 wycats@gmail.com

cc/ @rafaelfranca @etiennebarrie @casperisfine

@Edouard-chin Edouard-chin force-pushed the ec-bring-back-drawing-external-routes branch 2 times, most recently from eb1fead to 0641a56 Compare December 5, 2019 17:25
@rafaelfranca rafaelfranca requested a review from dhh December 5, 2019 17:34
@rafaelfranca
Copy link
Member

rafaelfranca commented Dec 5, 2019

@dhh mind to review the documentation to see if it talk about the trade-offs you want to be clear?

Copy link
Member

@dhh dhh left a comment

Choose a reason for hiding this comment

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

Besides this small nit, this is solid. Good to go!

Breaking up *very* large route file into multiple small ones:
-------------------------------------------------------

If you work in a large application with hundreds if not thousands of routes,
Copy link
Member

Choose a reason for hiding this comment

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

I'd say the cut-off is thousands. We have 500+ at Basecamp, and the split is def not worth it there.

Choose a reason for hiding this comment

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

We are including different routes on boot, based on some environment variables (like hostname), so for us it's not the total size, but the general separation of different customer routes which is important for us. So there are other factors too to consider, not only the size :)

@Edouard-chin Edouard-chin force-pushed the ec-bring-back-drawing-external-routes branch 3 times, most recently from 2d32687 to e47d9ec Compare December 6, 2019 02:33
@Edouard-chin
Copy link
Member Author

Thanks for the quick review. This should be good to go

= This feature existed back in 2012 rails@5e7d6bb
  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 <wycats@gmail.com>
@Edouard-chin Edouard-chin force-pushed the ec-bring-back-drawing-external-routes branch from e47d9ec to 33bf253 Compare December 6, 2019 13:20
@rafaelfranca rafaelfranca merged commit 758e4f8 into rails:master Dec 6, 2019
@Edouard-chin Edouard-chin deleted the ec-bring-back-drawing-external-routes branch December 6, 2019 14:16
rafaelfranca referenced this pull request Aug 26, 2020
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
@h0jeZvgoxFepBQ2C
Copy link

h0jeZvgoxFepBQ2C commented Aug 26, 2020

Could you maybe add a test which checks if a route concern works, if it's included in a previously loaded concern?

Like:

# config/routes.rb
Rails.application.routes.draw do
  draw(:admin)
  draw(:custom1)
  draw(:custom2)
end

# config/routes/custom1.rb
namespace :custom1 do
  concerns :admin
end

# config/routes/custom2.rb
namespace :custom2 do
  concerns :admin
end

# config/routes/admin.rb
concern :admin do
  resources :tests
end

I'm not sure, but i tried multiple splitting options in the last years, and many of them had a problem with concerns.

@rafaelfranca
Copy link
Member

It should work. You could try and report back if it doesn't but this mechanism is the same as defining those concerns in the same file.

shimbaco added a commit to annict/annict that referenced this pull request May 9, 2021
Rails 6.1から標準でroutesファイルを分割できるようになったため。
rails/rails#37892
@Ben-Fenner
Copy link

Thank you so much for re-implementing this! Our application has a very trim, optimized, 1,054-line routes file that I want to split up very badly! Hopefully we can get on Rails 6.1 soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants