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

Make the max-age key of the header cache-control configurable #181

Closed
wants to merge 1 commit into from

Conversation

cambad
Copy link
Contributor

@cambad cambad commented May 9, 2022

This pull request contains the changes regarding making the max-age key of the cache-control header during the store of the assets.

@taylorotwell
Copy link
Member

Why do you want to change this? The asset URLs change on each deploy.

@cambad
Copy link
Contributor Author

cambad commented May 9, 2022

Why do you want to change this? The asset URLs change on each deploy.

Oh sorry, this is my first pull request, I forgot to mention it but this is something I brought up to Vapor support who suggested I submit a Pull Request.

Here are more details. Precisely, JS and CSS served by Vapor will always have a unique URL for them, limiting the CDN and the user's browser to keep a copy of those for only 30 days means the same assets will need to be pulled every 30 days, even when no changes are done.

Since there is a unique URL, we should be able to set the maximum expiration time (1 year) which will tell the CDN (CloudFront) and the user browser to keep the asset for a long, long time, saving bandwidth and energy. 😄

Current 30 days expiration is seen by optimization tools like Pagespeed/Insights as a poor configuration since we are not efficiently leveraging on browser caching.

More details on optimization: https://web.dev/http-cache/#versioned-urls

@taylorotwell
Copy link
Member

Can we just change the default then? I don't think we need a config item for it?

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.

2 participants