Read digests of assets from manifest.yml if config.assets.manifest is on #2741

Merged
merged 1 commit into from Aug 30, 2011

Conversation

Projects
None yet
4 participants
Owner

guilleiguaran commented Aug 29, 2011

No description provided.

actionpack/lib/sprockets/railtie.rb
@@ -26,6 +26,12 @@ module Sprockets
end
end
+ if config.assets.manifest
+ if File.exist?(path = File.join(Rails.public_path, config.assets.prefix, "manifest.yml"))
+ config.assets.digests = YAML.load(File.read(path))
@tenderlove

tenderlove Aug 29, 2011

Owner

Change to YAML.load_file(path)

Contributor

rhulse commented Aug 30, 2011

This handles cases where there is no manifest, but I think there is an issue with an incomplete manifest that should be handed as well.

For example when upgrading an app there is a chance that some assets might not be moved into the assets path, meaning they won't get precompiled and Sprockets will get a hit and return a 404. There may be other edge cases where requests fall through the manifest check. Either way, this is a Bad Thing.

I think it is preferable to not send ay requests to Sprockets in production, and let the 404 be handled by the webapp in the normal way. Or perhaps just log a warning that 'asset path x' was requested but is not precompiled.

Owner

guilleiguaran commented Aug 30, 2011

@NZKoz change done

@rhulse I add an new option suggested by @NZKoz: guilleiguaran/rails@3455b51#commitcomment-558527

Is line 57 correct? Precompile happens on the production box (normally) so you would have to have a Javascript engine on there to compile.

Owner

guilleiguaran replied Aug 30, 2011

Heroku guys for example don't want to use a js engine in their boxes during runtime.

Ok, of course. Perhaps clarify? :-)

Contributor

rhulse commented Aug 30, 2011

Cool. Will test on staged production app as soon as these are merged in.

tenderlove added a commit that referenced this pull request Aug 30, 2011

Merge pull request #2741 from guilleiguaran/assets-manifest-yml
Read digests of assets from manifest.yml if config.assets.manifest is on

@tenderlove tenderlove merged commit 4a60a9e into rails:3-1-0 Aug 30, 2011

Owner

tenderlove commented Aug 30, 2011

Merged. Please give it a whirl!

Contributor

rhulse commented Aug 30, 2011

OK, initial quick test the app is running, YAML file is in place and being used, and a linked asset that I knew was not moved into a pipeline folder (and therefore not compiled) returns a 404 from the app. I am not seeing any Sprockets requests in the logs (so far), which is what is expected. Will test more in the morning. Others should test too, please :-)

+
+ # Create a manifest with the hashes of your assets when you run "rake assets:precompile".
+ # Use this if you don't have a JavaScript engine in your production servers
+ config.assets.manifest = true
@dhh

dhh Aug 30, 2011

Owner

This should not be exposed in application.rb. There's no reason to turn this off, so please just have it be silently on. In fact, I don't even see the point of this being a configuration at all.

- @assets.js_compressor = nil
- @assets.css_compressor = nil
+ @assets.manifest = true
+ @assets.precompile_only = false
@dhh

dhh Aug 30, 2011

Owner

Again, I don't see the point of having this be an option? If we're going to go with the manifest, why should it be optional? When the manifest is there, it should force precompile_only to be on. It doesn't make sense to ship both a manifest file AND allow live compilation.

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