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

cache compiled fingerprinted assets for 1y #139

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

Conversation

wenzowski
Copy link
Contributor

This sets the Cache-Control header to instruct the browser to cache everything served from the build folder for one year except for the three files marked volatile.

@wenzowski wenzowski mentioned this pull request Apr 19, 2016
@mattkrick
Copy link
Owner

Alrighty i'm gonna need a walkthrough on this one...
this is for setting headers on the client, right?

the prerender.js will never make it to the client, it's just used to build the page for SSR.
The assets.json is also used to build that page for SSR (including the standard HTML skeleton)

So that leaves us with the css file, right? If so, I think it may be worthwhile to just put a hash on the css filename in webpack & then we can always cache on the hash, which means we'll get a cache hit more often.

Ultimately, though, I'm thinking about converting the styles to react-look instead of css modules. 1 fewer HTTP request and all the speed benefits of css, although I can't yet figure out how to make it work with react-dom-stream.

@wenzowski
Copy link
Contributor Author

Good to know. PR updated.

This now

  1. denies access to server-only files
  2. sets Cache-Control:public, max-age=0 on css
  3. sets Cache-Control:public, max-age=31536000 on all other static files

By setting public and max-age we tell the browser that it may cache each fingerprinted chunk for up to one year.

In order to tell the browser that it must cache each chunk, we would still need to implement a Service Worker.

@wenzowski
Copy link
Contributor Author

This is setting http headers for the client and all intermediary caches to interpret. Let me know if a better walkthrough is necessary

@mattkrick
Copy link
Owner

so assuming we put a hash on the .css or move to an inline alternative (eg react-look), i think the PR is primarily for denying access to those 2 resources. Is there a reason we need to explicitly deny access to those resources? Alternatively, could we instead send those 2 to a serverBuild folder? I don't see any security problems with how it is currently, and my thought is that if blocking them is necessary, it should be done outside of runtime, but I don't have a strong opinion either way.

@wenzowski
Copy link
Contributor Author

The reason this PR exists is to add Cache-Control:public, max-age=31536000 headers wherever applicable. It wasn't immediately clear to me that prerender.js and assets.json were both server-only. By adding the access denied section that's explicit. So my thoughts on that are mostly about readability. It would be helpful to have the Cache-Control set correctly for css while those decisions are being made as well.

@wenzowski
Copy link
Contributor Author

wenzowski commented May 12, 2016

Alternatively, could we instead send those 2 to a serverBuild folder?

Yes, that would be just as readable, and would pull it out of runtime.

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