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 busting for icons & language files #8710

Merged
merged 7 commits into from Feb 20, 2019

Conversation

@bwindels
Copy link
Contributor

bwindels commented Feb 15, 2019

  • cache busting for icons
  • cache busting for language files.

React-SDK PR: matrix-org/matrix-react-sdk#2658

For i18n, I changed the copy-res.js script to generate content-hashed filenames and include those in languages.json, which then gets a content-hashed filename with file-loader.

Fixes #4136

@bwindels bwindels added the notready label Feb 15, 2019
@bwindels bwindels added this to In Progress in Workflow Feb 15, 2019
bwindels added 3 commits Feb 18, 2019
otherwise react-sdk can't build anymore with riot-web
in a specific location
@bwindels bwindels removed the notready label Feb 18, 2019
@bwindels bwindels requested a review from vector-im/riot-web Feb 18, 2019
@bwindels bwindels moved this from In Progress to In Review in Workflow Feb 18, 2019
…the unit tests in ci, where riot-web is a subdirectory of react-sdk
@bwindels bwindels force-pushed the bwindels/moarcachebustin branch from 900e603 to c2d1439 Feb 18, 2019
@bwindels

This comment has been minimized.

Copy link
Contributor Author

bwindels commented Feb 18, 2019

unit tests on react sdk are currently broken...

@jryans jryans self-assigned this Feb 18, 2019
Copy link
Member

jryans left a comment

Overall, this looks great! 😁 I am excited to see this merge.

// riot-web/webapp/i18n during build by copy-res.js
test: /\.*languages.json$/,
type: "javascript/auto",
use: [

This comment has been minimized.

Copy link
@jryans

jryans Feb 18, 2019

Member

Since there's only one loader for this test, you should be able to remove the use: [ ] part and fold the loader and options keys into the main object (see .wasm as an example).

fs.writeFileSync(dest + lang + '.json', JSON.stringify(translations, null, 4));
const json = JSON.stringify(translations, null, 4);
const jsonBuffer = Buffer.from(json);
const digest = loaderUtils.getHashDigest(jsonBuffer, "sha512", "base64", 7);

This comment has been minimized.

Copy link
@jryans

jryans Feb 18, 2019

Member

I think base64 includes /, which could get weird for things in file paths. Also, I'm not sure how much it actually impacts build time, but sha512 seems like overkill for this problem... We really only need the simplest thing that gives a different value when the file changes. (Also we're only keeping 7 bytes of it, so powerful hashes like sha512 are mostly doing work that is thrown out.)

For images via Webpack we're currently using [hash:7], which seems to equate to getHashDigest(buffer, null, null, 7), which will then default to md5 and hex, which seems fast, safe, and sufficient for the problem here.

@jryans jryans assigned bwindels and unassigned jryans Feb 18, 2019
@bwindels bwindels assigned jryans and unassigned bwindels Feb 20, 2019
@jryans
jryans approved these changes Feb 20, 2019
Copy link
Member

jryans left a comment

Great, this looks ready to go! Thanks for working on this. It should be quite helpful! 😁

@jryans jryans assigned bwindels and unassigned jryans Feb 20, 2019
@bwindels bwindels merged commit ad04e8b into develop Feb 20, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@jryans jryans moved this from In Review to In Test in Workflow Feb 20, 2019
@aaronraimist

This comment has been minimized.

Copy link
Contributor

aaronraimist commented Feb 22, 2019

Can this/should this be applied to config.json as well? #7241

@jryans

This comment has been minimized.

Copy link
Member

jryans commented Feb 22, 2019

Can this/should this be applied to config.json as well? #7241

Hmm, I don't think we can use the same approach for config.json, as it's not part of Riot's build process. I don't think we would want to entangle it in the build process either, as people might use other configuration management systems (Ansible, etc.) to produce and manage the file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.