documentcloud / jammit

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

Better mtime timestamps on generated asset package files #114

Closed charlessuh closed 13 years ago

charlessuh commented 13 years ago

Jammit's packager always sets the mtime of generated asset packages (the .css and .js files in public/assets) to Time.now.

Jammit could instead set the mtime to equal the mtime of the newest source file in the package, or if newer, the mtime of the Jammit configuration file.

When Jammit is run next time for a package, it can still detect if any of the source files or the configuration file has changed because it will still have a newer mtime than the generated asset package.

This will help greatly in environments where every deploy is sandboxed to its own directory, so there is no previous generated asset package files in public/assets.

This could be configurable via a switch if necessary.

Thanks!

jashkenas commented 13 years ago

I'm a bit confused about the use case here -- I think you'd need something pretty compelling in order to justify lying about the true mtime of the asset package.

What undesirable behavior were you encountering before this patch ... and what behavior are you seeing now that you have the patch installed?

charlessuh commented 13 years ago

The use case would be if you always deploy to a new directory (for example, a new deploy causes bits to be pushed to /var/deploys/1295632251/, and then /var/deploys/latest is symlinked to /var/deploys/1295632251/).

In this case, Jammit will generate new asset packages all set to the current timestamp, because the public/assets directory is always empty with a new deploy.

The only problem is that the browser caching is sub-optimal, as every deploy forces a new timestamp for all the asset packages and causes browsers to redownload because the "?timestamp" part of the URL has changed.

I think setting the mtime of the package to the mtime of the last source file in the package in order to ensure proper timestamp caching in this case might be a good reason to do it.