Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Make rake assets:precompile:nondigest optional #5379

Closed
wants to merge 8 commits into from
@davidjrice

(for applications with fully digested assets)

  • Improves speed of precompile significantly.
  • Therefore improving speed of deployments OOTB on Heroku etc.
  • assets:precompile:nondigest task still runs in Rake and warning issued
  • as Rake task still runs, rake task enhancements still possible
  • config.assets.nondigest_enabled setting disregarded if not present (backwards compatibility)
@davidjrice

This is actually in reference to a ticket I raised 5 months ago that is still open #3419

All of our applications on Rails 3 are fully using digested assets. With this patch it allows us to shave some time off of every deployment!

@josevalim
Owner
@davidjrice

@josevalim I know, there are separate tasks already. My motives for this pull request are not to workaround heroku limitations, but instead good defaults.

Most PaaS services I've seen are focusing on integration with rake assets:precompile as the default.

Sure on some platforms it means simply just changing a rake task that gets executed. On Heroku for example, I'd need to configure all of those apps with custom build packs (not so simple).

So, it would be great that this is just handled by a configuration setting in Rails so the only task need run is rake assets:precompile in all scenarios.

This is also great for extensions to the asset pipeline for example asset_sync where we are now relying on integrating with certain tasks in the pipeline compilation because of how Kernal.exec is being used.

In my opinion having one way of compiling assets makes things a lot simpler (to the end users). As it doesn't matter what my configuration is (wether I use digests or nondigest assets) my deployment processes are still the same.

@brianewing

+1 for good defaults

@josevalim
Owner
@guilleiguaran

please don't merge this (yet), I'm finishing the extraction of sprockets-rails from rails (probably I will finish it today)

@davidjrice

@guilleiguaran awesome!

@josevalim thanks.

I choose to implement this with Rails config vs ENV config based on the fact that the behaviour of rake assets:precompile is already influenced by the Rails config. I think this is the best approach as it means no matter where I run rake assets:precompile the result will be the same.

In my mind the config.assets.digest should determine wether to run precompile:primary or precompile:nondigest not both. The only reason we're doing both is to help people who haven't got all their asset references fixed to return the digested links in all aspects of their application... have I got that right? I've found that's great for bootstrapping a migration of a Rails 3.0 to 3.1 application. However it's really not helping people if they start using a CDN and are referencing nondigest assets unknowingly. It's much better if this fails fast and is fixed on a first deploy to a staging setup.

That's my opinion, and was my first port of call for implementing this but I thought it would be a much more difficult change, that's why I put together this patch which offers all the existing functionality and unobtrusively allows "taking off the training wheels" when you don't need them :)

Whilst it would be less preferable, I'd be up for reworking this to use ENV variables if that was the consensus!

Regarding asset_sync if I've made a mistake in our integration, I'm all ears. We're not actually hooking into both tasks: as of 0.3.0 (Rails 3.2.x) we're enhancing only the assets:precompile:nondigest task as it will always be ran and ran last.

Due to how the usage of Kernal.exec was introduced. We stopped enhancing the assets:precompile task but left it in the default Rake task for backwards compatibility with Rails 3.1.x releases to kick in only if the assets:precompile:nondigest task does not exist. Rather than sniffing Rails versions specifically.

I actually already fired a mail to Heroku for their thoughts on this. I totally agree, I'd love if their deployment strategies were a lot more customisable too.

@josevalim
Owner

The only reason we're doing both is to help people who haven't got all their asset references fixed to return the digested links in all aspects of their application... have I got that right?

The reason we are compiling both is because some times it is hard to access the digest version, like in emails, errors pages and other static pages. Indeed, referencing an asset without digest by accident is unfortunate, but removing the non digest version is the wrong answer to this problem.

That said, I don't feel strong about this feature but I am fine with an ENV variable since it feels less intrusive than adding a new option to config.assets.

@josevalim
Owner

Ok, I just got feedback from other Rails Core Members and some of them are actually interested in this feature. But we need a few changes to make everything work well. If you are up to the challenge we can work on this together, I can provide any guidance you need. Here are the concerns:

1) We would need to move 500.html, 404.html and friends to app/assets so they would be able to use the digested version of the assets;

2) I think development accepts both /assets/email.png and /assets/DIGEST-email.png. This is bad because if you accidentally use the non digest version in development, you will just catch the error in production;

If we can fix those two points, I believe we can change how this rake task behaves for Rails 4 to not include the non digest versions by default. If someone want the digest version, they can run rake assets:precompile:nondigest manually. Wdyt?

@davidjrice

Great. Can help out on this for sure.

Both those points, I agree. If you've any ideas on implementing would be good to hear. Any existing tickets out there with further info?

I'll look into 2) though it sounds easy enough.

Biggest concern would be 500.html and 404.html. I've had a think about how this should work...

  • Create a new assets directory app/assets/static
  • .html files in this directory are pre-processed / rendered (to allow .erb, .haml) extensions etc.
  • Generate defaults like app/assets/static/404.html.erb
  • files receive same scope for rendering as other assets to allow for digests
  • files are rendered to public/assets as normal
  • some special handling in the default error handler to render public/assets/404-DIGEST.html instead of public/404.html

Does that sound about right? not sure at the minute if there'd be much of an impact on routes or .htaccess files?

@soffes

@davidjrice that sounds amazing! I basically do that now, only all manually. This would be great.

@josevalim
Owner

Yes, the default error handler will need to be improved. Here is the current implementation: https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/public_exceptions.rb

For me, the biggest concern regarding 2) is: can it be an issue for many applications if the 404 page is actually hidden under assets/404-DIGEST.html? Well, if there is an issue they could use assets:precompile:nondigest to get both versions, I am just wondering if this would be the common case or the exception.

@jeremy: you commented that we could also include robots.txt at app/assets/static, but given that assets are compiled to public/assets, that wouldn't actually work. Any ideas in mind? Maybe we could tell sprockets that all files at app/assets/static should actually go to public/ ? Is it a good idea?

@jeremy
Owner

They should probably still generate digested assets in public/assets then symlink in from public/ so 404.html, robots.txt, etc always reference the latest file.

What do you think about a dir for "root" assets, like app/assets/static or app/assets/public?

@davidjrice

@jeremy @josevalim I've put together a first pass at getting this to work. I've slightly cheated and used a static middleware to lazily serve content in app/assets/public for development mode. Main bit I'm unsure about for now is how to get the static assets rendering with a .erb extension if present.

Would you suggest a custom middleware?

I still need to ensure referencing assets without a digest in emailers fails correctly.

@josevalim
Owner

@davidjrice the pull request is "dirty". Could you please rebase it?

davidjrice added some commits
@davidjrice davidjrice Make rake assets:precompile:nondigest optional (for applications with…
… fully digested assets)

* Improves speed of precompile significantly.
* Therefore improving speed of deployments OOTB on Heroku etc.
* assets:precompile:nondigest task still runs in Rake and warning issued
* as Rake task still runs, rake task enhancements still possible
* config.assets.nondigest_enabled setting disregarded if not present (backwards compatibility)
d1a8c2e
@davidjrice davidjrice Move all static assets from /public to /app/assets/public in Railties
Also remove stylesheets directory and make public an empty dir
636d014
@davidjrice davidjrice Generated public directory should have a .gitkeep, add static middlew…
…are to serve app/assets/public in development mode only
142d044
@davidjrice davidjrice Only apply app/assets/public middleware if config.serve_static_assets 0d8112d
@davidjrice davidjrice Symlink all assets generated from app/assets/public into public 1d0ea03
@davidjrice

@josevalim sorry, my bad, fixed this now.

@josevalim
Owner

@davidjrice thanks! Although I am skeptical about symlinks since they don't work on windows. We would need to check for alternatives on windows. Adding another middleware for app/assets/public also feels weird and as you said it will cause issues for rendering ERB files.

@jeremy any ideas on solving those issues?

@davidjrice
  • We could just copy the file to public on windows
  • The additional static middleware for app/assets/public is only a temporary POC hack :)
  • A middleware that would generate them from ERB if a .html.erb extension exists or simply serve the static file if it doesn't would be preferable... i'm just not sure of the best way of doing this yet.
@josevalim
Owner

@davidjrice Since it is inside app/assets, a developer would expect not only .erb to work, but also .haml and so on.

Ideally I would pipe the request to sprockets and let sprockets do its thing but afaik there is no way for us to tell sprockets to serve only assets for public.

davidjrice added some commits
@davidjrice davidjrice Only include app/assets/public static middleware when necessary. Impr…
…ove public exceptions middleware to render from app/assets/public as a fallback

* Include app/assets/public static middleware only (if config.serve_static_assets && config.assets.enabled && config.assets.compile) or in development mode
* Exception middleware checks public first and falls back to app/assets/public
975c261
@davidjrice davidjrice Improve public exception middleware, add attr for assets_public_path 067a73c
@davidjrice davidjrice When symlinking files under assets:precompile handle if there are alr…
…eady existing symlinks by unlinking them first
258ac20
@davidjrice

@josevalim yeah handling erb, haml etc. would be best.

Thought about integrating with sprockets, not sure where to start with that. (if anyone has any ideas I'd love to hear!)

It's possibly something that needs to go into sprokets-rails though?

@josevalim
Owner

Let's summon @josh and @sstephenson and see if they have any ideas/feedback about this.

@josh
Collaborator

The amount of flags and options in rails for the asset stuff is insane now.

Just put your "nondigest" stuff in public.

@davidjrice

@josh I know, the flag initially introduced at the start of this pull request was only intended to be an unobtrusive addition so as not to affect any existing functionality.

However after discussing with @josevalim the direction I am now heading towards is moving all the default rails static assets in public to within the asset pipeline so they may be precompiled. Being able to have these files as part of the application, pre-generated on deployment offers some great flexibility.

This could be taken a step further with the functionality I describe in this comment. Which would make rake assets:precompile:all and the new config I proposed unnecessary and rake assets:precompile would simply run either digest or nondigest only, depending on the setting of config.assets.digest. This would in turn simplify the assets:precompile task itself and not require shenanigans with Kernal.exec and would speed up execution.

@guilleiguaran

Closing this here since Sprockets integration was extracted from Rails edge and this couldn't be done in 3-2-stable. Please re-open your PR in sprockets-rails to continue the discussion (and please reference this PR)

@davidjrice
@guilleiguaran guilleiguaran reopened this
@homakov

@davidjrice just noticed. keep it up - it will also fix #6421

@guilleiguaran

Someone can elaborate a good reason to keep non-digested assets outside of public?

Like @josh I would prefer move non-digested stuff to public and compile only digested version of stuff in app/assets

@davidjrice

@guilleiguaran think you missed the whole point. If your read the above discussion again you'll see the goal is to generate files such as 404.html at precompilation time.

This allows usage of digested asset paths in 404/500 etc.
This also simplifies the whole digest / non-digest precompilation step and therefore reducing compilation time by 50%

As the last remnants of public/ are static files, however they need treated differently from assets as they are usually used on the server the rails app is served from, as opposed to assets which predominantly get shuffled to an asset host

@ndbroadbent

Hi, what do you think about #7866 as an alternative solution?

This patch means that only digest assets would be compiled, and non-digest assets are generated from those in only a few milliseconds. In that case, there would probably be no need to make them optional.

@davidjrice - I've also released these changes in a gem for Rails 3.2.8, at https://github.com/ndbroadbent/turbo-sprockets-rails3. I would be grateful if you could try it out and let me know if it solves your problem with long deployments.

@davidjrice

@ndbroadbent as mentioned, in #7866 I don't think it's an alternative solution. More of an extension to what I have here. I would definitely want a combination of the two in Rails 4!

The goal of this pull request should probably be restated that it is to Move the remaining rails public directory assets into the asset pipeline

@josh
Collaborator

The goal of this pull request should probably be restated that it is to Move the remaining rails public directory assets into the asset pipeline

No.

@route route referenced this pull request in rails/sprockets-rails
Closed

Last sprockets changes #9

@steveklabnik
Collaborator

Because sprockets-rails was moved out of master, there's no way this is going in. Therefore, I'm closing. Please re-submit to sprockets-rails and/or a backport fix to 3-2-stable.

@johnnyshields johnnyshields referenced this pull request in rails/sprockets-rails
Closed

Ability to skip non-digest assets? #97

@johnnyshields

Just a note for those who arrive here via Google: sprockets-rails (used in Rails 4.0.0+) no longer compiles non-digest assets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 14, 2012
  1. @davidjrice

    Make rake assets:precompile:nondigest optional (for applications with…

    davidjrice authored
    … fully digested assets)
    
    * Improves speed of precompile significantly.
    * Therefore improving speed of deployments OOTB on Heroku etc.
    * assets:precompile:nondigest task still runs in Rake and warning issued
    * as Rake task still runs, rake task enhancements still possible
    * config.assets.nondigest_enabled setting disregarded if not present (backwards compatibility)
  2. @davidjrice

    Move all static assets from /public to /app/assets/public in Railties

    davidjrice authored
    Also remove stylesheets directory and make public an empty dir
  3. @davidjrice

    Generated public directory should have a .gitkeep, add static middlew…

    davidjrice authored
    …are to serve app/assets/public in development mode only
  4. @davidjrice
  5. @davidjrice
  6. @davidjrice

    Only include app/assets/public static middleware when necessary. Impr…

    davidjrice authored
    …ove public exceptions middleware to render from app/assets/public as a fallback
    
    * Include app/assets/public static middleware only (if config.serve_static_assets && config.assets.enabled && config.assets.compile) or in development mode
    * Exception middleware checks public first and falls back to app/assets/public
  7. @davidjrice
  8. @davidjrice

    When symlinking files under assets:precompile handle if there are alr…

    davidjrice authored
    …eady existing symlinks by unlinking them first
This page is out of date. Refresh to see the latest.
View
14 actionpack/lib/action_dispatch/middleware/public_exceptions.rb
@@ -2,20 +2,28 @@ module ActionDispatch
# A simple Rack application that renders exceptions in the given public path.
class PublicExceptions
attr_accessor :public_path
+ attr_accessor :assets_public_path
def initialize(public_path)
@public_path = public_path
+ @assets_public_path = "#{Rails.root}/app/assets/public"
end
def call(env)
- status = env["PATH_INFO"][1..-1]
- locale_path = "#{public_path}/#{status}.#{I18n.locale}.html" if I18n.locale
- path = "#{public_path}/#{status}.html"
+ status = env["PATH_INFO"][1..-1]
+ locale_path = "#{public_path}/#{status}.#{I18n.locale}.html" if I18n.locale
+ path = "#{public_path}/#{status}.html"
+ locale_asset = "#{assets_public_path}/#{status}.html" if I18n.locale
+ asset = "#{assets_public_path}/#{status}.html"
if locale_path && File.exist?(locale_path)
render(status, File.read(locale_path))
elsif File.exist?(path)
render(status, File.read(path))
+ elsif locale_asset && File.exist?(locale_asset)
+ render(status, File.read(locale_asset))
+ elsif File.exist(asset)
+ render(status, File.read(asset))
else
[404, { "X-Cascade" => "pass" }, []]
end
View
19 actionpack/lib/sprockets/assets.rake
@@ -30,6 +30,18 @@ namespace :assets do
end
namespace :precompile do
+ def symlink_public_assets(digest=nil)
+ root = "#{Rails.root}/app/assets/public/"
+ Dir["#{root}*"].each do |p|
+ origin = p
+ path = p.gsub(root, '')
+ source = File.expand_path(File.join(Rails.public_path, ActionController::Base.helpers.asset_path(path)))
+ destination = "#{Rails.public_path}/#{path}"
+ File.unlink(destination) if File.exists?(destination) && File.symlink?(destination)
+ File.symlink(source, destination)
+ end
+ end
+
def internal_precompile(digest=nil)
unless Rails.application.config.assets.enabled
warn "Cannot precompile assets if sprockets is disabled. Please set config.assets.enabled to true"
@@ -54,6 +66,7 @@ namespace :assets do
:digest => config.assets.digest,
:manifest => digest.nil?)
compiler.compile
+ symlink_public_assets(config.assets.digest)
end
task :all do
@@ -71,7 +84,11 @@ namespace :assets do
end
task :nondigest => ["assets:cache:clean"] do
- internal_precompile(false)
+ if Rails.application.config.assets.nondigest_enabled || Rails.application.config.assets.nondigest_enabled.nil?
+ internal_precompile(false)
+ else
+ warn "Skipping assets:precompile:nondigest set config.assets.nondigest_enabled to true if required"
+ end
end
end
View
2  railties/guides/code/getting_started/config/application.rb
@@ -48,6 +48,8 @@ class Application < Rails::Application
# Enable the asset pipeline.
config.assets.enabled = true
+ # Enable automatic precompilation of nondigest assets
+ config.assets.nondigest_enabled = true
# Version of your assets, change this if you want to expire all your assets.
config.assets.version = '1.0'
View
2  railties/guides/source/asset_pipeline.textile
@@ -637,6 +637,8 @@ In +application.rb+:
<erb>
# Enable the asset pipeline
config.assets.enabled = true
+# Enable automatic precompilation of nondigest assets
+config.assets.nondigest_enabled = true
# Version of your assets, change this if you want to expire all your assets
config.assets.version = '1.0'
View
2  railties/guides/source/configuring.textile
@@ -139,6 +139,8 @@ Rails 3.1, by default, is set up to use the +sprockets+ gem to manage assets wit
* +config.assets.enabled+ a flag that controls whether the asset pipeline is enabled. It is explicitly initialized in +config/application.rb+.
+* +config.assets.nondigest_enabled+ a flag that controls whether the automatic precompilation of nondigest asset variants is enabled. It is explicitly initialized and set to true in +config/application.rb+.
+
* +config.assets.compress+ a flag that enables the compression of compiled assets. It is explicitly set to true in +config/production.rb+.
* +config.assets.css_compressor+ defines the CSS compressor to use. It is set by default by +sass-rails+. The unique alternative value at the moment is +:yui+, which uses the +yui-compressor+ gem.
View
3  railties/lib/rails/application.rb
@@ -234,6 +234,9 @@ def default_middleware_stack
end
if config.serve_static_assets
+ if (config.assets.enabled && config.assets.compile) || Rails.env.development?
+ middleware.use ::ActionDispatch::Static, "app/assets/public", false
+ end
middleware.use ::ActionDispatch::Static, paths["public"].first, config.static_cache_control
end
View
2  railties/lib/rails/generators/rails/app/app_generator.rb
@@ -96,7 +96,7 @@ def log
end
def public_directory
- directory "public", "public", :recursive => false
+ empty_directory_with_gitkeep "public"
end
def script
View
0  ...ators/rails/app/templates/public/404.html → .../app/templates/app/assets/public/404.html
File renamed without changes
View
0  ...ators/rails/app/templates/public/422.html → .../app/templates/app/assets/public/422.html
File renamed without changes
View
0  ...ators/rails/app/templates/public/500.html → .../app/templates/app/assets/public/500.html
File renamed without changes
View
0  ...rs/rails/app/templates/public/favicon.ico → ...p/templates/app/assets/public/favicon.ico
File renamed without changes
View
0  ...ors/rails/app/templates/public/index.html → ...pp/templates/app/assets/public/index.html
File renamed without changes
View
0  ...ors/rails/app/templates/public/robots.txt → ...pp/templates/app/assets/public/robots.txt
File renamed without changes
View
2  railties/lib/rails/generators/rails/app/templates/config/application.rb
@@ -64,6 +64,8 @@ class Application < Rails::Application
<% unless options.skip_sprockets? -%>
# Enable the asset pipeline.
config.assets.enabled = true
+ # Enable automatic precompilation of nondigest assets
+ config.assets.nondigest_enabled = true
# Version of your assets, change this if you want to expire all your assets.
config.assets.version = '1.0'
View
0  ...lates/public/stylesheets/.empty_directory → ...ils/app/templates/public/.empty_directory
File renamed without changes
View
28 railties/test/application/assets_test.rb
@@ -461,6 +461,34 @@ class ::PostsController < ActionController::Base ; end
assert_equal 0, files.length, "Expected application.js asset to be removed, but still exists"
end
+ test "nondigest assets are not precompiled if config.assets.nondigest_enabled set to false" do
+ app_file "app/assets/application.js", "alert();"
+ add_to_config "config.assets.compile = true"
+ add_to_config "config.assets.digest = true"
+ add_to_config "config.assets.nondigest_enabled = false"
+
+ quietly do
+ Dir.chdir(app_path){ `bundle exec rake assets:clean assets:precompile` }
+ end
+
+ files = Dir["#{app_path}/public/assets/application.js"]
+ assert_equal 0, files.length, "Expected application.js asset not to be generated, but was found"
+ end
+
+ test "nondigest assets are compiled if config.assets.nondigest_enabled not set" do
+ app_file "app/assets/application.js", "alert();"
+ add_to_config "config.assets.compile = true"
+ add_to_config "config.assets.digest = true"
+ add_to_config "config.assets.nondigest_enabled = nil"
+
+ quietly do
+ Dir.chdir(app_path){ `bundle exec rake assets:clean assets:precompile` }
+ end
+
+ files = Dir["#{app_path}/public/assets/application.js"]
+ assert_equal 1, files.length, "Expected application.js asset to be generated, but none found"
+ end
+
test "asset urls should use the request's protocol by default" do
app_with_assets_in_view
add_to_config "config.asset_host = 'example.com'"
Something went wrong with that request. Please try again.