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
Merged

Conversation

bwindels
Copy link
Contributor

@bwindels 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

…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 Compare February 18, 2019 18:06
@bwindels
Copy link
Contributor Author

unit tests on react sdk are currently broken...

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

@jryans jryans left a comment

Choose a reason for hiding this comment

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

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: [
Copy link
Collaborator

Choose a reason for hiding this comment

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

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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

@bwindels bwindels assigned jryans and unassigned bwindels Feb 20, 2019
Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

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
@aaronraimist
Copy link
Collaborator

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

@jryans
Copy link
Collaborator

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants