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

config.ru uses the effective Rack app #9669

Merged
merged 1 commit into from Mar 18, 2013

Conversation

Projects
None yet
5 participants
@senny
Member

senny commented Mar 11, 2013

We used to pass the Rails::Application subclass to #run.
The Rails server then called #to_app to convert that class to the
actual Rack application.

if you surround #run with a call to #map the server no longer
convertes the class to the instance and we end up with unnecessary
delegation calls on every request.

@senny

This comment has been minimized.

Show comment
Hide comment
@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Mar 11, 2013

Member

Per this we should probably instantiate the app in config.ru, rather than reference Rails.application.

Member

tenderlove commented Mar 11, 2013

Per this we should probably instantiate the app in config.ru, rather than reference Rails.application.

@carlosantoniodasilva

View changes

Show outdated Hide outdated railties/lib/rails/commands/server.rb
@@ -42,10 +42,6 @@ def initialize(*)
set_environment
end
def app
@app ||= super.respond_to?(:to_app) ? super.to_app : super
end

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Mar 11, 2013

Member

Won't this thing call super twice on every call to app?

@carlosantoniodasilva

carlosantoniodasilva Mar 11, 2013

Member

Won't this thing call super twice on every call to app?

This comment has been minimized.

@senny

senny Mar 12, 2013

Member

it would yes.

@senny

senny Mar 12, 2013

Member

it would yes.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Mar 12, 2013

Member

@tenderlove I think we can make this change before moving away from the singleton. Instantiating in config.ru will have further consequences. I'd like to get this applied first. It behaves exactly the same as it did but does not take different paths wether you wrap it with map or not.

Member

senny commented Mar 12, 2013

@tenderlove I think we can make this change before moving away from the singleton. Instantiating in config.ru will have further consequences. I'd like to get this applied first. It behaves exactly the same as it did but does not take different paths wether you wrap it with map or not.

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Mar 15, 2013

Member

👍 as a stopgap. Worried that removing #app will break existing apps that don't have the updated config.ru.

Member

jeremy commented Mar 15, 2013

👍 as a stopgap. Worried that removing #app will break existing apps that don't have the updated config.ru.

config.ru uses the effective Rack app
We used to pass the Rails::Application subclass to #run.
The Rails server then called #to_app to convert that class to the
actual Rack application.

if you surround `#run` with a call to `#map` the server no longer
convertes the class to the instance and we end up with unnecessary
delegation calls on every request.

jeremy added a commit that referenced this pull request Mar 18, 2013

@jeremy jeremy merged commit 0053c21 into rails:master Mar 18, 2013

@claudiob

This comment has been minimized.

Show comment
Hide comment
@claudiob

claudiob Jan 13, 2015

Member

@senny Do you think it's the right time to deprecate this?

@senny Do you think it's the right time to deprecate this?

This comment has been minimized.

Show comment
Hide comment
@senny

senny Jan 14, 2015

Member

@claudiob I think the timing would be good. How would you deprecate it?

Member

senny replied Jan 14, 2015

@claudiob I think the timing would be good. How would you deprecate it?

y-yagi added a commit to y-yagi/rails that referenced this pull request Aug 7, 2017

Deprecate support of older `config.ru`
Since Rails 4.0, `config.ru` generated by default uses instances of
`Rails.application`.  Therefore, I think that it is good to deprecate
the old behavior.

Related: #9669
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment