-
Notifications
You must be signed in to change notification settings - Fork 4
Add cache busting to remove need for Cloudflare purging #15
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
Conversation
Deploying openhomefoundation-org-website with
|
Latest commit: |
8505f6e
|
Status: | ✅ Deploy successful! |
Preview URL: | https://11b2ec3b.openhomefoundation-org-website.pages.dev |
Branch Preview URL: | https://cache-bust.openhomefoundation-org-website.pages.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 1 out of 2 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- src/_includes/base.liquid: Language not supported
Co-authored-by: Copilot <[email protected]>
@@ -5,6 +5,8 @@ module.exports = (eleventyConfig) => { | |||
eleventyConfig.addPlugin(require("./config/css-config.js")); | |||
eleventyConfig.addPlugin(require("./config/js-config.js")); | |||
|
|||
// --------------------- Global Data ----------------------- | |||
eleventyConfig.addGlobalData("CACHE_KEY", Buffer.from(String(new Date().valueOf())).toString('base64').replaceAll("=", "")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be automated without the need to remember to add this data to each place it's wants.
eleventyConfig.addTransform(
"cache-buster-assets",
function (content, outputPath) {
const staticDir = resolve(eleventyConfig.dir.input, "src", "assets");
if (outputPath.endsWith(".html")) {
return content.replace(/="\/assets\/([^"]+)"/g, function (_, matcher) {
const filepath = matcher.split("?")[0];
const timestamp = statSync(
resolve(staticDir, filepath)
).mtime.getTime();
return `="/assets/${filepath}?v=${timestamp}"`;
});
}
return content;
}
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I am on the fence about this. I like having control over what is busted. E.g. images, fonts don't need to be busted on each build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They will not; this used file-modified timestamp, not build timestamp.
Which means that unless the file changes, the v
param will stay the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here is a variant for css/js only:
eleventyConfig.addTransform("cache-buster", function (content, outputPath) {
const staticDir = resolve(eleventyConfig.dir.input, "src", "static");
if (outputPath.endsWith(".html")) {
return content.replace(
/="\/static\/([^"]+\.(css|js))"/g,
function (_, matcher) {
const filepath = matcher.split("?")[0];
const fileStat = statSync(resolve(staticDir, filepath));
const timestamp = fileStat.mtime.getTime();
return `="/static/${filepath}?v=${timestamp}"`;
}
);
}
return content;
});
And again, as it uses mtime, it will only update when actually needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can tell, using the modified time won't work since the dist build is made from scratch on each build, meaning this will purge every time. Or am I getting this wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The time is based on the source, not the dist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh yes. I knew I was getting something mixed up
@ludeeus Can we get this current PR implementation approved and work on iterating it in the future. Currently having no implementation causes more problems. Currently, I don't have the time to spend on this but if you can get it working your way, feel free to implement |
I will not approve something I do not agree with, so someone else will have to do that. |
Yeah, I understand. Thanks for the other PR. |
No description provided.