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

turn off cache busting #14

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

turn off cache busting #14

wants to merge 25 commits into from

Conversation

sandfox
Copy link

@sandfox sandfox commented Sep 16, 2017

No description provided.

JamieDixon and others added 25 commits August 15, 2017 10:12
Update from upstream repo facebookincubator/create-react-app
Update from upstream repo facebookincubator/create-react-app
Modify prod build

Move package over to @connected-home

Downgrade jest and babel-jest to support V3

Undo commented template section. Should work now that were referencing react-scripts as part of CRA rather than a folder in our project

Delete yarn.lock file and use npm for this proj instead

Remove fs package that was added by us for some reason

Add HivehomeWebappFaviconsWebpackPlugin with proper config

Remove path package from devDependencies
Adjust a few bits so that Jest works properly with V3

Add property to jest config for matching jest files in this lower version of jest
…the end

Bump favicon plugin ref to 2.0.0

Change favicon config to use ios rather than iphone key

Bump versions
all our existing stuff uses sha256 to
- reduce chance of collisions
- make the content more properly content-addressable
…v and prod. Added browser-detect to template paths. Exit early in locales when no directory path specified (template)
…ign with V3 requirements but this breaks e2e
Added items to the template to match the webpack build process for de…
Add build number to webpack env vars
Reset babel-jest and jest versions to match upstream
@@ -375,7 +375,7 @@ module.exports = {
// used to populate the caches, to ensure the responses are fresh.
// If a URL is already hashed by Webpack, then there is no concern
// about it being stale, and the cache-busting can be skipped.
dontCacheBustUrlsMatching: /\.\w{64}\./,
dontCacheBustUrlsMatching: /./,

Choose a reason for hiding this comment

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

Nice! Do we need this property at all?

Copy link
Author

Choose a reason for hiding this comment

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

yes, without it defaults to cache busting everything.

Choose a reason for hiding this comment

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

I'm not 100% sure on this one. Why is this a problem for us and not for any other CRA app?

Copy link
Author

Choose a reason for hiding this comment

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

CRA defaults assume that people don't have the correct (or any) set cache-control (and related) headers properly on the web server where the static content comes from.
In those cases the cache busting is the "safe option" for making sure that fresh content always gets the browsers at the cost nothing being cached.

We have the correct headers set, and therefore can turn off the cache busting so that browser (and intermediate server) caches are used.

Copy link
Author

Choose a reason for hiding this comment

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

actually to be more correct it's SWPrecacheWebpackPlugin that assumes people's cache headers are all wrong.
CRA itself does shorter version of hashing like we do for the static assets, and sets this value to /\.\w{8}\./ which basically turns off cache busting for the static assets (because if those assets change, so does their name and therefore no risk of the them being in a cache.)

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