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 static asset caching (with cache busting) #627

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

Conversation

colinrotherham
Copy link
Contributor

Hi @joelanman @igloosi @NickColley @alexnewmannn,

This pull request builds on top of #615. I can't stop noticing the flicker/flashes of fonts and assets since it was first raised.

Features:

  1. Static asset URLs now have cache-busting paths
  2. Static assets add Cache-Control: public, max-age=31536000 (one year)
  3. Each restart of nodemon or Heroku deployment busts the cache
  4. No configuration required

Before

/assets/images/govuk-apple-touch-icon.png
/public/stylesheets/application.css

After

/assets/jnymnqyn/images/govuk-apple-touch-icon.png
/public/jnymnqyn/stylesheets/application.css

To achieve this, the prototype kit now injects a new $govuk-assets-path into GOV.UK Frontend.

I've followed the assetPath and assetUrl convention as seen here:
https://github.com/alphagov/govuk-frontend/blob/master/src/template.njk

I've written to a persistent ./app/version.txt file so we can read in the current cache version but I don't mind changing this approach if needed. Maybe an environment variable instead?

@colinrotherham colinrotherham force-pushed the static-caching branch 2 times, most recently from bb88e20 to cff4b57 Compare November 1, 2018 13:46
@colinrotherham
Copy link
Contributor Author

Rebased and changelog added

@colinrotherham colinrotherham force-pushed the static-caching branch 2 times, most recently from 879782e to 8a2552b Compare November 1, 2018 15:23
@colinrotherham
Copy link
Contributor Author

Rebased with v8.3.0


// Inject Sass variables
const variables = () => {
cacheId = cacheId || fs.readFileSync(path.resolve('./app/version.txt'), 'utf-8').trim()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be more robust to fingerprint the assets themselves?

For example https://www.npmjs.com/package/gulp-fingerprint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup this would be the best approach! Significantly more work 😊

E.g. we'd need to update govuk-font-url() and govuk-image-url() in GOV.UK Frontend to add a hash/version path: https://github.com/alphagov/govuk-frontend/blob/c809239e05c34c5adb578e7420090478a169a1e5/src/tools/_font-url.scss

Otherwise the URL will be cached for a year without cache-busting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @NickColley, I keep rebasing this fork just in case there's any appetite to use it?

We're using it across four different prototypes and the speed difference in IE11 is pretty huge. No flickering or flash of missing fonts per page load.

Do you still have concerns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NickColley Given this some more thought.

To use per-file fingerprinting (e.g. gulp-rev) by adding hashes to all included filenames, we'd need to dynamically patch:

app/views/includes/head.html
app/views/includes/scripts.html

Since the prototype kit doesn't follow the usual app/src ==> dist "copy on build" step that others do this would be quite tricky to do without big build changes and watch tasks.

The git-rev gulp plugin can write to a manifest file (to track files renamed with added hashes) but then the running Node.js application would have to be aware of this to include the correct CSS/JS file.

Have you had any other thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we'd need to use a helper function in the Nunjucks view that references the manifest generated by gulp-rev?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does that not feel too tied in to specific tooling? It's what I started doing, wanted to check first. Leave it with me then

@joelanman
Copy link
Contributor

Thanks for this. We'll be looking at the asset pipeline of the Prototype Kit soon, so we're not sure yet this is definitely the approach we'll be taking. As this is a user-facing change (they need to change their paths) we're not going to merge this just now.

@colinrotherham
Copy link
Contributor Author

colinrotherham commented Nov 9, 2018

Hi @joelanman, thanks.

Just a note though, this pull request has full backwards compatibility.

I put this little bit in so URLs are routed as if the cache-busting path isn't there:

if (cacheId) {
  req.url = req.url.replace(cachePath, '/')
}

i.e. Old paths still work, no changes required.

@colinrotherham
Copy link
Contributor Author

colinrotherham commented Nov 9, 2018

I've followed the guide here with an old kit:
https://govuk-prototype-kit.herokuapp.com/docs/backwards-compatibility

Once upgraded, all v6 kit includes get the new maxAge header too because I've adjusted the legacy asset_path to get the cache-busting directory.

For those following the instructions in the guide to replace /public/ to /public/v6/ will continue to work as before but will need asset_path to upgrade to the new caching.

@colinrotherham
Copy link
Contributor Author

Rebased with v8.4.0

@edwardhorsford
Copy link
Contributor

edwardhorsford commented Feb 7, 2019

I'm finding the flash of fonts really distracting - I'm guessing users won't notice it so much, but if it could be fixed, that would be great.

Looking through some old prototypes they did not have this flash.

Edit: it also gives prototypes the appearance of being slower, which isn't ideal.

@colinrotherham
Copy link
Contributor Author

colinrotherham commented Feb 8, 2019

@edwardhorsford You can use this fork if you like for prototypes?

As @NickColley mentioned, with this pull request every "deployment" to Heroku will have a clear cache. Ideal for prototypes, but not the best for production—it's something I'd like to revisit again.

By all means use this as a starting point though.

Or see how I've added the maxAge header and add this to your own? Something reasonably low like 30 minutes might work well for now https://github.com/alphagov/govuk-prototype-kit/pull/627/files#diff-78c12f5adc1848d13b1c6f07055d996eR117

@edwardhorsford
Copy link
Contributor

@colinrotherham I'm not sure asset caching is the issue with the font flashing. Looking in the inspector, assets are already cached - yet we're I'm still seeing the flash of font. It's almost like something is loading the fallback in first then deciding to load NTA.

@NickColley
Copy link
Contributor

@edwardhorsford you're seeing an intentional loading behaviour but because it's not cached correctly it happens every time rather than just the first time.

@colinrotherham
Copy link
Contributor Author

Rebased again with master, but this now needs more work to apply it to #613

At the moment, all extensions have no maxAge cache either.

@NickColley
Copy link
Contributor

We spoke about this issue in our triage session, we recognise this is annoying but don't have the time to prioritise it right now.

For this to be merged we'd want a fingerprinting approach to ensure it's robust long term and is more consistent with how we do this in the Design System website.

I've spoken to Colin and he's suggested leaving this open since it's useful for his team to start their prototypes from. 👍

Cache is busted for every deployment
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.

None yet

5 participants