Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@schneems it currently breaks several tests, working on this now...

I helped @mikhailov with his first PR to Rails. Excited to see new contributors! 🌠

(ref #23510)

Seems like a good idea to me 👍 -- thanks for opening the PR!

Setting aside some implementation specifics, my main concern here is in having knowledge of /assets creeping into places that really shouldn't know about it.

If we believe it's safe to bypass this, can't we just move the sprockets middleware further up the stack? As far as I know, it's currently below things like the reloader because assets can invoke application code. 😕

can't we just move the sprockets middleware further up the stack?

I thought about that, but in fact sprockets works as a mounted Rails engine, not as a middleware.
https://github.com/rails/sprockets-rails/blob/master/lib/sprockets/railtie.rb#L166

We could change that if we really wanted. We could also make this a configurable value so any engine could say "hey I don't need code reloaded"

I think the MD point is cause for caution assets can and do use routes at minimum. In some cases people use them with models or other code.

Yeah, assets can be arbitrary ERB templates.

@fxn, I tried to create app/assets/javascripts/application.js.erb with the followed content:

<%= "hi, I'm ERB code 1+1 = #{1+1}" %>

it works as expected, ERB is being processed correctly, no rails server restart required on assets change. As I understand *.rb files are autoloded and required to be reloaded on change, run_callbacks (:prepare, :cleanup) help to perform clean reload, no?

@kirs right, what about Sprockets::Server.call ?

@schneems @matthewd totally agree on conditional code reload mechanism, however it tends to be a big change and touch vast majority of existed middleware codebase. If possible I'd like to focus on the following method that does more than it probably should for static assets (correct me if I'm wrong):

    def prepare! #:nodoc:
      run_callbacks :prepare if validated?
    end

Yes.

Indeed, with the evented monitor in Rails 5 the reloader test is probably even cheaper than this code, because it involves a method call that just checks a boolean flag instead of string inclusion (not that it would matter).

The evented monitor is going to be enabled by default in new applications (this is still not in master).

@fxn that's really great to know! I'm happy to test this new feature, where can I get it from? #22254 ? The reason for that PR is improvement in complexity O(1) compare to O(n) for only static assets which can be 80% of total number of requests. N represents number of Ruby classes/modules in application. On a small code base (< 2K LOC) n is very low so no difference in serving assets speed compare to large code base.

group :development do
  ...
  gem 'listen', '3.0.5'
end

After bundle install I ran few tests with siege (siege -b http://localhost:3000/assets/application.js)

without this PR:
Transaction rate: 1.94 trans/sec

with this PR:
Transaction rate: 17.52 trans/sec

This is the same as #10291. I still think that the best action to take is #10291 (comment)

@rafaelfranca fair enough, however this is not exactly the same, adding another layer of middleware is no-go, total agree with you!

#10291 highlights another kind of issue when number of assets is too high compare to an application's LOC. Rather than discussing performance in terms of slower/faster or temporarely patching an issue it's worth to discuss how complex serving static assets for large size application is and what can we do about it. Complexity analysis shows that amount of Ruby code slows down static assets in O(n) so it can be expensive on large code base.

As @fxn mentioned evented monitor involves a method call that just checks a boolean flag (makes a big sense for Ruby classes), is there anything we can do about static assets as well, regexp by file extension in order to bypass Reloader callbacks?

mddleware ActionDispatch::Reloader is probably not the best place to put the code, but let me share the reason behind this PR below. The measurements (/assets/application.js) provided by siege against 1 unicorn worker and Ruby 2.3.0p0, Rails 4.2.5.1:

Have you tried with the evented monitor in Rails 5? In addition to listen you need to set the class, that was added in 7223596.

For the archives: it was tried and didn't made the gap smaller.

You could provide this as a gem pretty easily. In the railtie you replace this reloader middleware with your own custom middleware. It could inherit from the reloader and optionally call super based on the path. I would make the endpoints that it skips configurable so people could add or remove them.

@fxn @schneems Good idea! Btw, what about configurable middleware here: https://github.com/rails/rails/blob/master/railties/lib/rails/application/default_middleware_stack.rb#L63-L65 we should have an access to config.assets.prefix from here, right?

The idea is that the middleware shouldn't have to have access to any of that. You would have an api like

# config/initializers/my_middleware.rb
MyMiddleware.skip_reload_route_match << Regexp.escape(Rails.config.assets.prefix)

So anyone could add anything. You will have access to that config value via rails engines, but i'm not sure if you get it before or after it config/environments/.rb

@schneems this sounds like a plan! I like an idea to make some kind of abstraction similar to what we have for config.autoload_paths and be able to extend when required but the only thing doesn't let me sleep, what else can it be? I mean what kind of non asset request can bypass class reload mechanism?

@schneems what about actionpack/lib/action_dispatch/railtie.rb update similar to recent PR 60b53e9#diff-182aa02288995070b335f57216738b92R42

ActionDispatch::Reloader.skip_reload_route_match = config.assets.prefix

/cc @matthewd

You don't need to change anything in Rails to do what I suggested, make a gem that has a middleware that replaces the current middleware. Are you running into a problem there?