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

Add timestamp to layouts for cachebusting #254

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

whostolemyhat
Copy link
Contributor

This allows layouts to access a millisecond timestamp which can be used for cachebusting (adding a unique string to static assets to get around long caching times in the browser).

Notes

  • I used Value::Str since Value::Num only accepts f32 - the timestamp is i64 so casting to f32 trims the number to the point where it's not unique.
  • There's a bit of a dance in the test since the timestamp is generated when the build command is run - this means the target to compare with needs to be built when cargo test runs.
  • It might be better to add date instead of timestamp and then format in the layout file - this would give users more flexibility since they can access the Date object, but would mean each asset would have to format the date individually.

Copy link
Member

@epage epage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows layouts to access a millisecond timestamp which can be used for cachebusting (adding a unique string to static assets to get around long caching times in the browser).

So this is adding a timestamp of when a page was generated so we can have a unique value each time we might want to bust caches?

First off, I'll admit (yet again) I'm not a web dev and don't know what the best practices are for cache busting (I've read up on the basic theory).

Would the general recommendation be

  • To use the generated-time feature you are adding?
  • Write a monotonically non-increasing integer in place of the timestamp and manually bump it? This would be a bit tedious, so some sub-options include
    • Side-wide attribute so people can do <link rel="stylesheet" href="/css/main.css?v={{ cachebuster }}">
    • Add support for data files (serde makes it easy for us to even support csvs for this) with all the assets listed out in a data file for easy update throughout the site (so you'd have a link like <link rel="stylesheet" href="{{ assets.main_css }}">)
  • Have cobalt checksum asset files and automatically insert the value, so you'd get links like <link rel="stylesheet" href="{{ assets.css.main_css.link }}">)

The downside to the manual integer work is that it requires people to be disciplined about updating the number when they update an asset.

The downsides to using generation-time is that it is less obvious and flushes the caches more often.

The downside of the checksum solution is that it is a bit more involved of a solution.

Thoughts?

@@ -0,0 +1,7 @@
extends: default.liquid
---
This is my Index page!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If your goal is to test the timestamp feature, is there a reason to have posts, or having the timestamp in the file you extend rather than having it in your actual index.liquid?

Boiling down tests to cover only the feature in question helps to make the tests more stable and reduces churn when something unrelated changes that causes either minor tweaks (like syntect making slight layout changes) or major ones (us breaking compatibility as we work towards 1.0).

@@ -176,12 +176,15 @@ pub fn build(config: &Config) -> Result<()> {
.collect();

trace!("Generating other documents");
let timestamp = Value::Str(UTC::now().timestamp().to_string());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used Value::Str since Value::Num only accepts f32 - the timestamp is i64 so casting to f32 trims the number to the point where it's not unique.

liquid-rust/#88 will require us to have integer values. So in the future we can change this.

@epage
Copy link
Member

epage commented Jul 4, 2017

Note: overall, I think it might be useful to have some kind of "generated-time" value but I'd expect it to parallel the eventual "published-time" and "last-modified-time" attributes which would all be in date formats. If it doesn't exist, we could add a liquid filter to turn a datetime into an integer value.

So if we decide a checksum solution is best but you want a solution now, reframing this as "generated-time" timestamp might work out.

@whostolemyhat
Copy link
Contributor Author

You're right, this would end up breaking the cache each time a build is run, so if you're creating a post a day it'd have a pretty detrimental affect on caching (the opposite of what we want!)

I think it makes sense to turn this into the generated-time attribute then parse it in the layouts for a quick solution; the better option for cachebusting would be to hash/checksum each asset so only updated ones are affected. This would be more complex to do since we'd need to map each asset and insert the hashed filename in each document but would be better to avoid breaking caching for unchanged assets.

@epage
Copy link
Member

epage commented Jul 6, 2017

the better option for cachebusting would be to hash/checksum each asset so only updated ones are affected. This would be more complex to do since we'd need to map each asset and insert the hashed filename in each document but would be better to avoid breaking caching for unchanged assets.

cobalt already copies over every asset, so it can easily get a checksum as it goes. Building up a liquid::Object to store it all in isn't too bad. The main annoyance is updating the pipeline to handle assets before everything else. Hopefully my planned refactors will help with it. I'm about done with the second step of my refactor (first was file walking, second is document parsing). Next up is config file handling. I think after that is fixing up the pipeline. This is not a quick process. See #257 for some of the care I'm taking in tracking everything.

@epage epage mentioned this pull request Nov 2, 2017
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