Make Rails static server exclude /public/assets in development ENV #6421

Closed
homakov opened this Issue May 21, 2012 · 38 comments

Projects

None yet
@homakov
Contributor
homakov commented May 21, 2012

Pretty common pitfall for newbies in Sprockets. Everyone will get this error once. )

If you once run rake assets:precompile and then start rails in development mode your JS and CSS code will be included twice(with sprockets just-in-time generated and with precompiled application.js which is in public/assets). It looks very foggy to them, you can see all assigned DOM events are doubled - 2 alerts, 2 appends etc. Difficult to figure out what the problem is unless you stumbled upon this a lot of times.

Also it's not always a good way to make sure rake assets:clean - I don't want to remove stuff from public/assets, it will break my production installation!

So, let's just stop looking up static stuff at "public/assets" when Rails.env.development?

Here is working patch for actionpack/lib/action_dispatch/middleware/static.rb

    def call(env)
      case env['REQUEST_METHOD']
      when 'GET', 'HEAD'

        path = env['PATH_INFO'].chomp('/')
        unless Rails.env.development? and path[0..7] == '/assets/'
          if match = @file_handler.match?(path)
            env["PATH_INFO"] = match
            return @file_handler.call(env)
          end
        end
      end

      @app.call(env)
    end

the change will allow to swap dev/prod environments immediately, without bothering about already precompiled assets and rm -rf public/assets/*

Of course it should be opt-out-able in development.rb and I need to refactor it in more ruby-way.

What do you think about this? We will accomplish seamless working dev and prod environments + help guys who's new with asset pipeline :)

@dmathieu
Contributor

+1, I like this.

@dmathieu
Contributor

One thing though. It'd be better not to directly use Rails.env.development? in the code.
Perhaps Rails.application.config.consider_all_requests_local instead, so we can have that behavior on an other environment if necessary.

@pixeltrix
Member

I'm not getting double inclusion in my apps - can you push an app to GH that shows this? However if I have precompiled assets they are preferred over dynamic assets so there's an argument to be had that running in development mode should ignore public/assets but I don't think it's right to do it in the static middleware. Also we'd need to take account of the possibility that people aren't running the asset pipeline and have something else at public/assets - that'd be really confusing!

Ideally this should be handled by sprockets-rails - you may want to create a issue there.

@homakov
Contributor
homakov commented May 21, 2012

You both are pretty right

  1. not only development should be affected. test too
  2. dynamic assets must be preferred - it's the point.
  3. we do not need this if assets.enabled = false, of course!

Just run rake assets:precompile and than rails s -e development - you will see path /assets/applicaton.js - it includes ALL JS you have so JS is included twice.

ANd it has nothing to do with sprockets. Sprockets are responsible for compiling assets. Rails static server is responsible for serving. We just need to stop serving "public/assets" and serve only dynamic assets.

@pixeltrix
Member

Just run rake assets:precompile and than rails s -e development - you will see path /assets/applicaton.js - it includes ALL JS you have so JS is included twice.

Nope - just the one copy. Is it just JS or CSS as well?

The Rails static server is just serving up what's in public/assets/application.js - are there two <script> tags or are you seeing two lots of content in the one file? If the latter check the public/assets/application.js file - are there two copies in there? If so, Sprockets must be including two copies somehow.

@homakov
Contributor
homakov commented May 21, 2012

Example :

    = javascript_include_tag :application
generates
    <script src="/assets/search/templates.js?body=1" type="text/javascript"></script>
    <script src="/assets/action1/on_ready.js?body=1" type="text/javascript"></script>
    <script src="/assets/application.js?body=1" type="text/javascript"></script>

but /assets/application.js?body=1 returns ALL precompiled and compressed code if you ran rake assets:precompile. Very uncomfortable.

@pixeltrix
Member

Got it - I'm not getting because of this in development.rb:

config.assets.debug = false

One workaround when developing is to only compile digested assets - that way the non-digested versions will fall back to dynamically served assets.

@homakov
Contributor
homakov commented May 21, 2012

@pixeltrix the idea sounds good but it is not a solution.
I want to compile all(not only digested) assets - I use them in production.
I still want to use dynamic assets - when I am in development.

I don't want these two envs to intersect. Prod uses /public/assets, Dev uses dynamic assets. They don't depend on each other at all. This is why the only way is to exclude 'public/assets' from looking up pathes in development environment.

@pixeltrix
Member

I want to compile all(not only digested) assets - I use them in production.

I was talking about compiling assets for local use when running as production, you'd still run the full compile on your production server - unless you check public/assets in to your repository, in which case it wouldn't work. Also only suggested as a temporary workaround until we address this.

I don't want these two envs to intersect. Prod uses /public/assets, Dev uses dynamic assets. They don't depend on each other at all. This is why the only way is to exclude 'public/assets' from looking up pathes in development environment.

Patching the static file server just seems the wrong way to go about it - we need to change the precedence of how the routes are handled in development, so that Sprockets gets to serve the files before hitting the file system.

@homakov
Contributor
homakov commented May 21, 2012

@pixeltrix hmm making sprockets' routes more preferred than static server will be a good solution. I don't know how to implement it in current middleware system - static handler goes the very first. Looking forward others' opinions!

@homakov
Contributor
homakov commented May 21, 2012

Also, thanks for reminding about config.assets.debug. Why is it true by default? First time I've seen "expanded lines" I was surprised because in production assets are compressed in 1 file.

@pixeltrix
Member

I'm guessing config.assets.debug is true by default because it makes it easier to use tools like Firebug or Web Inspector.

As for how make the static handler come after routes - we could turn it from a middleware into a rack app and append it to the routeset using mount so that everything else has precedence. You wouldn't normally run it in production, but if you did you could prepend it so that static files had precedence there.

@sikachu
Member
sikachu commented May 22, 2012

I myself never seen the double asset serving myself, as it would compile asset on-the-fly if I'm on the development environment. I think it's always acting that way, unless something has changed in the latest release.

Let me try to reproduce your issue first. So you just running assert precompile task and serving the site in dev env?

@sikachu
Member
sikachu commented May 22, 2012

I mean, if it's double include, the problem maybe occur somewhere else - not the static middleware alone.

@homakov
Contributor
homakov commented May 22, 2012

@sikachu just static middleware goes first, before app routes.

  1. I don't want to clean up assets. I have precompiled public/assets/application.js which I don't want to delete.
  2. I have a line in <head> <script scr = assets/application.js> which makes browser to include that static file - JS is doubled
  3. I want to make static server ignore assets folder so all request would be treated as dynamic
@pixeltrix
Member

@sikachu what''s happening is that when config.assets.debug = true then each of the individual asset pipeline files are included along with the main application.js or application.css (as these may included definitions as well). However if you have precompiled assets existing in public/assets (or wherever your asset path is) then the static file server in development will return the compiled application.js or application.css instead of the debug version from Sprockets because it's higher in the middleware stack than the router.

The problem exists because we compile non-digested assets as well as the digested forms so they clash with the Sprockets urls. Some people check their assets into VC so we need to find a solution that works without having to remove the compiled assets folder - which is why I suggest putting the static server after the router, either just by moving it's position in the middleware stack or turning into a rack app and mounting it in the routeset.

@homakov
Contributor
homakov commented May 22, 2012

Exactly @pixeltrix what I wanted to say.

I'm afraid of moving routing before static - if we have a lot of static requests it will run through all route set very often so will take more memory. But current state is also bad - it's hard to figure out what's wrong if you get this error. I don't know what's best option here.

@pixeltrix
Member

Probably be a millisecond or two slower but shouldn't take any more memory - unless the router is leaking memory.

@sikachu
Member
sikachu commented May 22, 2012

I can see that problem now. That's why I was against compiling non-digested asset in the first place ... sigh.

I think the best way would be putting the sprocket middleware in front only when the config.consider_all_requests_local is true. In that case if serve_static_assets is enable in production it won't hurt the performance.

@pixeltrix
Member

@sikachu Sprockets is a Rack application, not a middleware - that's why I suggested making the static file server a Rack application as well so it could be mounted after Sprockets.

@homakov homakov closed this Aug 20, 2012
@route
Contributor
route commented Sep 7, 2012

Guys why is this closed? The problem is still here, I think.

@steveklabnik
Member

It was @homakov's issue, so he chose to close it. Not sure. Do you still see the problem somewhere?

@homakov homakov reopened this Sep 7, 2012
@route
Contributor
route commented Sep 7, 2012

@steveklabnik I just know that it wasn't fixed, I think it should be open.

@rafaelfranca
Member

I don't think we should fix this since is a programmer error, but if anyone want to fix please go ahead

@route
Contributor
route commented Sep 11, 2012

I can imagine two cases of deploying assets to production:

  1. Assets are compiled on production running rake task
  2. Assets are compiled on dev machine and production server just pull them

This issue describes the second case, when assets should be in git index, but it causes a headache in dev mode.

@ndbroadbent
Contributor

+1, this is a very annoying problem. I like the original idea of patching actionpack/lib/action_dispatch/middleware/static.rb, but with development? swapped for consider_all_requests_local (as suggested by @sikachu):

unless Rails.application.config.consider_all_requests_local && path.starts_with?('/assets/')
...
@josh
Member
josh commented Oct 15, 2012

Definitely no to any changes to static.rb. First, static should be coupled to any sort of public conventions. Also many people have apache or nginx server out of public and avoid hitting rails. The static middleware is meant to mirror the behavior of simple static serving.

@allomov
allomov commented Dec 5, 2012

+1 for changes in this direction. Here is connected issue #5145.

@pixeltrix pixeltrix added a commit that closed this issue Dec 6, 2012
@pixeltrix pixeltrix Invert precedence of content in ActionDispatch::Static
This commit inverts the precedence in ActionDispatch::Static so that
dynamic content will be served before static content. This is so that
precompiled assets do not inadvertently get included when running in
development mode - it should have no effect in production where static
files are usually handled by the web server.

Closes #6421
c59734f
@pixeltrix pixeltrix closed this in c59734f Dec 6, 2012
@josh
Member
josh commented Dec 6, 2012

Why don't you guys just output your assets to separate directories if you intend to check them into git.

public/assets/production won't clash in dev mode. No code changes, you can do this now.

@pixeltrix
Member

I've reverted the commit that fixed this in af73e3c, however I'm leaving it and #5145 closed since according to Josh you are all 'noobs' who shouldn't be using Rails.

@danbob
danbob commented Dec 7, 2012

I have to say +1 for the revert. I was originally looking forward to this fix, but a simple rm -rf public/assets at the end of my deploy script fixed the problem for me. And yesterday I was trying to run in production mode on my local machine to test digested assets, but because of the fix I was getting No Route errors for the compiled application.js and application.css files.

@homakov
Contributor
homakov commented Dec 7, 2012

my initial issue was about explaining people why they get 'static' version in favour of dynamic from sprockets.
Not about checking it in git for sure, josh. And I was OK to close it a few times, but friends of mine tell me they get this error all the time(when forget to purge /assets folder).
I am OK to just forget it. While it behaves normally, NOOBS will consider it as a bug. FAKIN NOOBS YEA?!11

@pixeltrix
Member

I think the way we're going to address it is by adding a warning on boot if config.assets.compile = true, config.serve_static_assets = true and File.exist?('public/assets/application.css') || File.exist?('public/assets/application.js') with a recommendation to compile to public/assets/production if you wish to check in assets to version control.

@pixeltrix
Member

If there is anyone that does want to have stuff in public/assets and would prefer dynamic content over static content in development then here's one way of doing it:

# Put this at the end of your routes.rb
if Rails.env.development?
  app = ActionDispatch::Static.new(
    lambda{ |env| [404, { 'X-Cascade' => 'pass'}, []] },
    Rails.application.config.paths['public'].first,
    Rails.application.config.static_cache_control
  )

  mount app, :at => '/', :as => :public
end
@maciej
maciej commented Dec 28, 2012

@pixeltrix either I'm doing something wrong or the workaround you've provided does not work for me. I have a static public/assets/application.js and one in app/assets/javascripts. With the snippet in the bottom of my routes.rb Rails is still serving the static one.
Here is my stack:

  • latest Google Chrome (cache emptied)
  • JRuby 1.7.0
  • Rails 3.2.9
  • Torquebox 2.1.2

Anyways… still I'll just move to using public/assets/production.

@pixeltrix
Member

@maciej have you disabled the default static server? e.g:

config.serve_static_assets = false

in your config/environments/development.rb file.

@maciej
maciej commented Dec 28, 2012

@pixeltrix nope… thanks for the tip!

Anyways, eventually I opted for precompiling the assets on the production servers.

@pixeltrix
Member

@maciej that's the best way to go and there are some significant performance gains to come in Rails 4 that will help.

@sodabrew sodabrew referenced this issue in sodabrew/puppet-dashboard Apr 9, 2013
Closed

(maint) only add chart if it's not there #199

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