-
Notifications
You must be signed in to change notification settings - Fork 153
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
chore(perf): Avoid conflicting hints to preload and defer initial JS #14992
base: main
Are you sure you want to change the base?
Conversation
fc5f892
to
13cf683
Compare
@@ -24,7 +21,6 @@ export function linkHeadersMiddleware( | |||
`<${WEBFONT_URL}/ll-unica77_regular.woff2>; rel=preload; as=font; crossorigin`, | |||
`<${WEBFONT_URL}/ll-unica77_medium.woff2>; rel=preload; as=font; crossorigin`, | |||
`<${WEBFONT_URL}/ll-unica77_italic.woff2>; rel=preload; as=font; crossorigin`, | |||
...linkHeaders, |
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're addressing the warnings, but do you remember that wild decrease in stats that we saw from calibre? It was due to adding this.
Are we sure here?
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.
Things have been crazy today and i haven't been following closely, but are you sure that removing defer
in collectAssets doesn't address all the things in the best way?
We're loading so much js. We want to hint that.
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.
To be honest, no I'm not sure. My tests were pretty clear that the js should stay deferred ("lab score" went from 70 to 55 otherwise). This change eliminated the warnings, but didn't score any better.
If it were the middle of the week, I'd say let's try it for an official Calibre run or 2. Since it's not, let's wait and revisit next week. I think the more impactful change will be to preload the main image (#14974), ahead of these assets.
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 can run a couple manual snapshots if that helps anything, but i see what you mean in terms of getting the weeks average. Either way!
#1277 Bundle Size — 8.95MiB (0%).13cf683(current) vs fbc4882 main#1264(baseline) Warning Bundle contains 14 duplicate packages – View duplicate packages Bundle metrics
|
Current #1277 |
Baseline #1264 |
|
---|---|---|
Initial JS | 3.65MiB |
3.65MiB |
Initial CSS | 0B |
0B |
Cache Invalidation | 39.33% |
39.33% |
Chunks | 103 |
103 |
Assets | 106 |
106 |
Modules | 5834 |
5834 |
Duplicate Modules | 530 |
530 |
Duplicate Code | 4.03% |
4.03% |
Packages | 266 |
266 |
Duplicate Packages | 13 |
13 |
Bundle size by type no changes
Current #1277 |
Baseline #1264 |
|
---|---|---|
JS | 8.81MiB |
8.81MiB |
Other | 143.36KiB |
143.36KiB |
Bundle analysis report Branch joeyAghion/hint Project dashboard
Generated by RelativeCI Documentation Report issue
This addresses warnings from tools like DebugBear about:
Currently, we specify in
Link:
headers and<link>
tags that initial JS bundles should be preloaded. But the actual<script>
tags specify they should bedefer
red. After this PR, the initial JS chunks won't be preloaded, but will be downloaded only once the<script>
tags are encountered, and executed whenever the browser decides to executedefer
red scripts.This definitely addresses the DebugBear warnings. The waterfalls seem the same... script execution (~1s in throttled simulations!) happens at the end. It also frees bandwidth to fetch the main image, which is necessary for the LCP event. The thing I'm not 100% sure about is: does this delay later events like dom-ready or TTI? It didn't in my analyses but seems like it could be a possibility.