Skip to content
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

Warning about middleware not auto-reloading #17187

Merged
merged 1 commit into from
Oct 8, 2014

Conversation

Bounga
Copy link
Contributor

@Bounga Bounga commented Oct 6, 2014

Add a section in the guide to explain that Rails can't auto-reload
a middleware on code change.

Fix #16806

@@ -99,6 +99,14 @@ To find out more about different `rackup` options:
$ rackup --help
```

### Development and auto-reloading

Once loaded, there's no way for Rails to auto-reload a middleware on changes.
Copy link
Member

Choose a reason for hiding this comment

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

This sentence is strange. What about 'Middlewares are loaded once and are not monitored for changes.'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you're right. I'm going to change this.

@thedarkone
Copy link
Contributor

👍

However thinking about it, for development, you could use something like this:

class MiddlewareTester
  def initialize(app, tested_middleware_const_name, require_path = nil)
    @app = app
    @tested_middleware_const_name = tested_middleware_const_name
    @require_path = require_path
  end

  def call(env)
    load(@require_path) if @require_path # might be brittle
    tested_middleware_const_name.constantize.new(@app).call(env)
  end
end


Middlewares are loaded once and are not monitored for changes.

Middleware changes will not be automatically picked up, you will have to restart the server for changes to be reflected in the running application.
Copy link
Member

Choose a reason for hiding this comment

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

Now we are repeating ourself, we just said that they are not monitored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we must say explicitly that the server has to be restarted for the changes to be effective. So what to do? Combine both sentences?

Copy link
Member

Choose a reason for hiding this comment

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

I'm talking about

are not monitored for changes.

and

Middleware changes will not be automatically picked up

@Bounga
Copy link
Contributor Author

Bounga commented Oct 6, 2014

Why does Travis build failed on a guide update?

@Bounga
Copy link
Contributor Author

Bounga commented Oct 7, 2014

So it is not good enough to be included in the guide?

@seuros
Copy link
Member

seuros commented Oct 7, 2014

Travis sometime timeout, please add [ci skip] in your commit message when sending doc.
Please squash your commit now. Look good to merge.

@seuros seuros added the docs label Oct 7, 2014

Middlewares are loaded once and are not monitored for changes. You will have to restart the server for changes to be reflected in the running application.

As a middleware developer it means that you'll have to restart your test application on every single change.
Copy link
Member

Choose a reason for hiding this comment

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

Is not this paragraph redundant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe yes, I wanted to emphasis on the fact that as a Rails developer of a middleware you have this strong constraint.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove that.

@seuros
Copy link
Member

seuros commented Oct 7, 2014

@Bounga Please rebase/squash commits..

@Bounga Bounga force-pushed the middleware_reloading_explaination branch from f018aa7 to 0a86ac3 Compare October 8, 2014 07:26
@Bounga
Copy link
Contributor Author

Bounga commented Oct 8, 2014

@seuros Sorry about the rebase/squash lag. I was new to that so I had to dig about the good way to do it.

The guide (http://guides.rubyonrails.org/contributing_to_ruby_on_rails.html#contributing-to-the-rails-code) is missing a little tip about the command git rebase -i which is how to select the number of commits to rebase: git rebase -i HEAD~4 for example.

Add a section in the guide to explain that Rails can't auto-reload
a middleware on code change.

Fix rails#16806

[ci skip]
@Bounga Bounga force-pushed the middleware_reloading_explaination branch from 0a86ac3 to 094a7ce Compare October 8, 2014 07:36
seuros added a commit that referenced this pull request Oct 8, 2014
Warning about middleware not auto-reloading
@seuros seuros merged commit c736aaa into rails:master Oct 8, 2014
@seuros
Copy link
Member

seuros commented Oct 8, 2014

Thank you.

@Bounga Bounga deleted the middleware_reloading_explaination branch October 8, 2014 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editing i18n causes middleware that references constants to explode
4 participants