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

Use content hashing for font and image URLs #8159

Merged
merged 5 commits into from
Jan 18, 2019

Conversation

jryans
Copy link
Collaborator

@jryans jryans commented Jan 18, 2019

This adds new builds steps to Webpack in order to transform font and image URLs by appending a content hash to the output location so that we can then use a long cache lifetime while still adding new versions easily.

Since fonts and images shouldn't change very often, they remain outside of the "bundles" used for CSS and JS. Instead, only the output file name is changed. The directory structure is unchanged. This does make it a bit harder to prune old images compared to the bundles, but most likely there won't be too many versions of images anyway.

This change includes support for the Status theme and images in HTML templates. For more details on how image paths work going forward, see matrix-org/matrix-react-sdk#2460.

Fixes #7932
Depends on matrix-org/matrix-react-sdk#2460

This adds a `file-loader` rule to the Webpack build so that any requests for
image resource will be output into the app's output directory, but with an extra
content has appended so that we can safely use a long cache lifetime.

The CSS and SCSS rules are also changed to use `css-loader` so that any `url`
inside is automatically processed by the new image rule above.
This means that themes which include `light/css/_base.scss` (currently Dark and
Status) won't be forced to have Light's font-faces included.  This only really
matters for Status, which uses different fonts throughout.
Adds a `$res` SCSS variable set to the path from the root SCSS file to the `res`
directory.

This is a different base path than previously used in CSS URLs (it goes up 3
directories instead of 2), because Webpack will now be resolving images relative
to the root SCSS file, so the path corresponds to a source tree location,
instead of a path in the build output tree.

Defining this variable has two main goals:

* URLs are a bit easier to read
* The path can be overridden, which is needed for riot-web themes like Status
Expands the image build process to also support the right paths when used in
HTML templates.
This allows home.html to remain a regular HTML file without templating while
still having a copy of the rooms directory icon.
@jryans
Copy link
Collaborator Author

jryans commented Jan 18, 2019

Removing review request for now. There are still a few issues with Karma tests to fix.

@jryans jryans requested review from a team and removed request for a team January 18, 2019 15:09
@jryans
Copy link
Collaborator Author

jryans commented Jan 18, 2019

Karma tests do pass here, but with some warnings when images load. I don't think we need to block this work for that, as we can improve it later and it has no impact on the tests themselves.

@turt2live turt2live self-assigned this Jan 18, 2019
@turt2live turt2live assigned jryans and unassigned turt2live Jan 18, 2019
@jryans jryans merged commit d71e84b into experimental Jan 18, 2019
@NotAFile
Copy link
Contributor

NotAFile commented Jan 18, 2019

This has broken most of the image loading for me, fyi.

This might just be because /experimental isn't adjusted to these changes yet?

@jryans
Copy link
Collaborator Author

jryans commented Jan 18, 2019

This has broken most of the image loading for me, fyi.

This might just be because /experimental isn't adjusted to these changes yet?

Thanks, please file an issue with screenshot so we can triage and fix.

There's already #8164, but if you see something else, then please do file.

@NotAFile
Copy link
Contributor

There's already #8164

yep, that looks pretty much like what's happening for me!

@t3chguy t3chguy deleted the jryans/versioned-img-urls branch May 12, 2022 09:03
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