-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
perf: optimize fonts by using woff2 instead of ttf #26554
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
3ffb029
to
8fe8185
Compare
// can test this by uncommenting it, running with --zip, and then unzipping | ||
// the resulting zip file. If it is still broken the unzip operation will | ||
// show an error. | ||
// '.ttf', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't have any ttf files anymore, so this comment about uncommenting is no longer true, so i've removed it.
attributes: { as: 'font', crossorigin: true }, | ||
// preload our own fonts, as other fonts use fallback formats we don't | ||
// want to preload | ||
test: /fonts\/\.(?:woff2)$/u, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this adds preload
link tags to home.html
, popup.html
, and notification.html
. Neat!
@@ -323,7 +333,7 @@ const config = { | |||
}, | |||
// images, fonts, wasm, etc. | |||
{ | |||
test: /\.(?:png|jpe?g|ico|webp|svg|gif|ttf|eot|woff2?|wasm)$/u, | |||
test: /\.(?:png|jpe?g|ico|webp|svg|gif|woff2|wasm)$/u, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change wasn't really necessary, as the removed font formats (ttf and eot) would still be parsed and handled by webpack, but they'll just end up in a different place with a weird name. I still like getting them out of our logic here though, just because its slightly simpler to read.
@@ -0,0 +1,68 @@ | |||
declare module 'postcss-discard-font-face' { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh, this package didn't have types so I made them myself.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #26554 +/- ##
========================================
Coverage 70.01% 70.01%
========================================
Files 1410 1410
Lines 49130 49130
Branches 13739 13739
========================================
Hits 34398 34398
Misses 14732 14732 ☔ View full report in Codecov by Sentry. |
8fe8185
to
8fbe3af
Compare
Builds ready [8fe8185]
Page Load Metrics (70 ± 6 ms)
Bundle size diffs
|
8fbe3af
to
3986be9
Compare
Quality Gate passedIssues Measures |
Builds ready [3986be9]
Page Load Metrics (74 ± 8 ms)
Bundle size diffs
|
Great update! Any way we can see the performance improvements from these changes? |
Quality Gate passedIssues Measures |
Builds ready [673091f]
Page Load Metrics (1804 ± 119 ms)
Bundle size diffs
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is a medium CVE?Contains a medium severity Common Vulnerability and Exposure (CVE). Remove or replace dependencies that include known medium severity CVEs. Consumers can use dependency overrides or npm audit fix --force to remove vulnerable dependencies. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
} | ||
|
||
@font-face { | ||
font-family: 'Euclid Circular B'; | ||
font-style: normal; | ||
font-weight: 500; | ||
src: url('#{$font-path}/Euclid/EuclidCircularB-Medium.ttf') format('truetype'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The newest font package has a EuclidCircularB-Medium-WebS.woff2
and a EuclidCircularB-Medium-WebXL.woff2
. It doesn't have a EuclidCircularB-Medium.woff2
, so I just went with the WebXL version because all of our other fonts also use that version.
Builds ready [88519e9]
Page Load Metrics (1820 ± 51 ms)
Bundle size diffs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reason for change: woff2 is has been supported by Firefox and Chrome since circa 2015, so it's safe to use. It's designed specifically for use on the web. It is a smaller format than an equivalent ttf (which we were using prior to this PR).
This PR isn't substantial. On my machine it shaves only 8 milliseconds off of our popup's load time (from ~60.5ms to 52.5ms - using #26555 as the baseline).