Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only set max expires header on digested assets #64

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

seanlinsley
Copy link

No description provided.

@rhomeister
Copy link
Contributor

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
Copy link
Author

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
Copy link
Collaborator

@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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Author

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
Copy link
Collaborator

@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
Copy link
Collaborator

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

@seanlinsley
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants