search

capistrano-plugins / capistrano-unicorn-nginx

Capistrano tasks for automatic and sensible unicorn + nginx configuration
MIT License
175 stars 81 forks source link

Only set max expires header on digested assets #64

Open seanlinsley opened 9 years ago

rhomeister commented 9 years ago

Thanks for your PR. Could you give some explanation about the changes you've made? Why is this useful and/or which problem does this solve?

seanlinsley commented 9 years ago

If you have requests for the undigested version (because a third party expects the asset to be permanently available), setting max expires prevents you from propagating an updated version of the file.

griffithac commented 7 years ago

@seanlinsley are you still using this gem and is this still a concern of yours? I am new to the project and trying to see what issues I can resolve or close.

griffithac commented 7 years ago

Users that what this behavior should be able to modify the default templates as described in the documentation below. If you (@seanlinsley) don't feel this is adequate to address most users needs, please let me know.

https://github.com/capistrano-plugins/capistrano-unicorn-nginx/wiki/Template-customization

seanlinsley commented 7 years ago

Yep we're still using this gem. The code we have has changed from 32 characters to 64 characters, though, because of a change in Sprockets.

  location ~* "-[a-z0-9]{64}\.(png|gif|jpg|jpeg|css|js)$" {
    gzip_static on;
    expires max;
  }

The whole point of having a default template is to provide common defaults. Anyone who's using Rails without an asset CDN should have this setting, and it doesn't hurt anyone who is using a CDN.

Why couldn't this be merged? (after the 32 -> 64 change)

seanlinsley commented 7 years ago

Oh and the more important argument that I'd posted here before: if you vendor any third-party assets but don't bundle them up into your application.js, the existing expires max will prevent you from being able to update those assets.

griffithac commented 7 years ago

@seanlinsley I will test you code on my production app tomorrow. It looks find to me from what I see here. And I get your point. I think @rhomeister hasn't had a lot of time to maintain the gem. If he wants to chime in that would be great. If not I will see about moving forward with this after some testing. I was just granted push rights today, so I am still getting acquainted with the gem.

griffithac commented 7 years ago

@seanlinsley are you serving up fonts from the asset pipeline?

seanlinsley commented 7 years ago

Looks like we are. So this PR should be updated to also match fonts.

Another option would be to update this PR so that it doesn't care what the file extension is. That would make it more future-proof, for sure.