documentcloud / jammit

Industrial Strength Asset Packaging for Rails
http://documentcloud.github.com/jammit/
MIT License
1.16k stars 197 forks source link

asset tag timestamps #27

Closed ghazel closed 14 years ago

ghazel commented 14 years ago

I see that Jammit.asset_url takes mtime optionally, but I do not see asset tag timestamps in the urls to packages or to urls in CSS files.

In addition, you could ues the Rails asset tag timestamp cache easily. Something like:

# Generates the server-absolute URL to an asset package.
def self.asset_url(package, extension, suffix=nil)
  source = "/#{package_path}/#{filename(package, extension, suffix)}"
  # ActionView::Helpers::AssetTagHelper#
  rewrite_asset_path(source)
end
ghazel commented 14 years ago

Ignore that!

I do see asset tag timestamps on stylesheet and javascript links, just not on the first page load. That makes sense to me now.

I'm still not seeing them on the image urls in CSS, but those do not pass through asset_url()...

ghazel commented 14 years ago

This works:

diff --git a/lib/jammit/compressor.rb b/lib/jammit/compressor.rb
index 0b46b95..3ed161b 100644
--- a/lib/jammit/compressor.rb
+++ b/lib/jammit/compressor.rb
@@ -7,6 +7,8 @@ module Jammit
   # all stylesheets, with all enabled assets inlined into the css.
   class Compressor

+    include ActionView::Helpers::AssetTagHelper
+
     # Mapping from extension to mime-type of all embeddable assets.
     EMBED_MIME_TYPES = {
       '.png'  => 'image/png',
@@ -110,7 +112,7 @@ module Jammit
         File.read(css_path).gsub(EMBED_DETECTOR) do |url|
           ipath, cpath = Pathname.new($1), Pathname.new(File.expand_path(css_path))
           is_url = URI.parse($1).absolute?
-          is_url ? url : "url(#{rewrite_asset_path(ipath, cpath, variant)})"
+          is_url ? url : "url(#{construct_asset_path(ipath, cpath, variant)})"
         end
       end
       stylesheets.join("\n")
@@ -143,10 +145,11 @@ module Jammit
     # Return a rewritten asset URL for a new stylesheet -- the asset should
     # be tagged for embedding if embeddable, and referenced at the correct level
     # if relative.
-    def rewrite_asset_path(asset_path, css_path, variant)
+    def construct_asset_path(asset_path, css_path, variant)
       public_path = absolute_path(asset_path, css_path)
       return "__EMBED__#{public_path}" if embeddable?(public_path, variant)
-      asset_path.absolute? ? asset_path.to_s : relative_path(public_path)
+      source = asset_path.absolute? ? asset_path.to_s : relative_path(public_path)
+      rewrite_asset_path(source)
     end

     # Get the site-absolute public path for an asset file path that may or may
jashkenas commented 14 years ago

Alright -- thanks for spearheading this one. I had mixed feelings on whether or not we should be messing with your CSS contents by adding in the rails timestamp -- since that's not what RAILS_ASSET_ID does by default. But I think it is a pretty nice feature, so thanks for making it happen.

I've merged your branch and made some tweaks:

Take a look at the changes, and let me know if you're happy with where things stand, and we can push out an update to the gem.

documentcloud commented 14 years ago

Quick ping -- I'd still like to push out a new version of the gem. Have you had a chance to take a look?

ghazel commented 14 years ago

Wow, somehow I completely missed your response. Checking it out now...

ghazel commented 14 years ago

Looks good!

documentcloud commented 14 years ago

Alright, this is now out with Jammit 0.4.4 -- closing the ticket.

ghazel commented 14 years ago

Thanks!