-
Notifications
You must be signed in to change notification settings - Fork 341
MAINT - Defer FontAwesome script to improve Lighthouse performance score #2209
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
MAINT - Defer FontAwesome script to improve Lighthouse performance score #2209
Conversation
"All checks have passed" 🎉 |
Lighthouse performance scores for Kitchen Sink pages after this PR:
|
Consistent success of the link-check CI job depends on #2207 |
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.
Done self-reviewing
"/api.html", | ||
"/blocks.html", | ||
"/generic.html", | ||
"/lists.html", |
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.
images.html is explicitly excluded because the performance score is likely the result of Picsum taking too long to load images
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 a is a good observation, and it is likely this is the root cause.
docs/_static/pydata-icon.js
Outdated
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.
Moved into custom-icon.js.
The main reason I did this was that when I tried implementing this with two separate JS files, deferred fontawesome.js, and conf.py like so:
html_js_files = [
("custom-icon.js", {"defer": "defer"}),
("pydata-icon.js", {"defer": "defer"}),
]
Then for some reason only one of the two custom icons would load correctly on localhost:8000.
I really do not understand why it was working locally before without defer but then failed with defer.
But I also didn't want to spend a long time trying to debug this and digging into the FontAwesome source code. Plus it's better anyway to have fewer network requests for the custom icons (each js file in html_js_files is a separate network request). So I decided to consolidate them into one file and update the docs.
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.
I was never able to figure out why, before this PR, the icon files could be split in two and everything worked, whereas after this PR, splitting the icon files into two results in only the first one working.
That said, I wrapped the consolidated file in various setTimeouts—0, 1, 10, 100, 1000, and 10000 ms—to try to rule out the possibility of a strange race condition.
Well, there probably is some kind of strange race condition, but it probably has to do with subsequent calls to FontAwesome.library.add()
. (One clue that this is the case is that if you wrap the code in pydata-icon.js on main
in a long enough timeout, say 100 ms, then the icon fails to load.) Based on my testing, so long as add() gets called once with all of the custom icon definitions, instead of for each icon definition, then it seems like everything works fine.
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.
So this PR probably also makes custom icon loading more robust, yay!
docs/conf.py
Outdated
@@ -257,7 +257,9 @@ | |||
# so a file named "default.css" will overwrite the builtin "default.css". | |||
html_static_path = ["_static"] | |||
html_css_files = ["custom.css"] | |||
html_js_files = ["pydata-icon.js", "custom-icon.js"] | |||
html_js_files = [ | |||
("custom-icon.js", {"defer": "defer"}), |
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.
See note in PR description. Since this PR changes fontawesome.js to defer, it must also change custom-icon.js (or any code that depends on fontawesome.js to defer)
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.
Moved this to custom-icons to reinforce the idea that all custom icons should be loaded in a single file
Just noting that in the course of working on this PR I have noticed some of the CI jobs taking a really long time (over 1 hour) and never finishing. |
... | ||
] | ||
|
||
.. versionchanged:: v0.17.0 |
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.
if we don't release this PR with v0.17.0, then we will need to update this to the actual version that incorporates the changes in this PR.
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.
I have added this PR to the v0.17.0 milestone but should I also label it as block-release
?
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.
No need to add as block-release
, the PR seems pretty much ready now.
This reverts commit 43b4277.
docs/_static/custom-icons.js
Outdated
// This file is referenced in the docs, so if you make changes to it be sure to | ||
// update the docs too :) |
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.
Since this file is not minified on our docs site, I assume it's not minified on theme-adopter sites. So I'm going to remove all unnecessary comments and just link to the docs page instead.
You can see it's not minified by visiting the URL: https://pydata-sphinx-theme.readthedocs.io/en/latest/_static/custom-icon.js?v=74687d30
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.
Thanks @gabalafou this looks fantastic to me and it is a huge improvement re performance!
I think we are ready to merge so will go ahead with this.
"/api.html", | ||
"/blocks.html", | ||
"/generic.html", | ||
"/lists.html", |
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 a is a good observation, and it is likely this is the root cause.
... | ||
] | ||
|
||
.. versionchanged:: v0.17.0 |
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.
No need to add as block-release
, the PR seems pretty much ready now.
This pull request:
Defer
Warning
When this pull request gets released in the next version of the PyData Sphinx Theme, it will require theme users to update their custom icon entries in
html_js_files
to include thedefer
attribute (see the update to the docs page in this pull request).Without the defer attribute, the entire page has to wait for the FontAwesome script to be downloaded and executed. Lighthouse identified this script as the worst offender. With the defer attribute, the page should be able to load faster.
Excluding images.html
The reason I'm excluding images.html from Lighthouse is because it contains images that are hotlinked from Picsum. I think that this is the reason the Lighthouse performance score comes in too low (below 80). Because when I examined the Lighthouse report for that page, I saw the main reason the page failed was because of a slow Largest Contentful Paint, and when digging into that, the report showed the following table:
I assume that most of that load delay has to do with hotlinking from Picsum and nothing that we can fix without modifying the Kitchen Sink pages, which we're not supposed to do since those are generated from a script.
Using the Lighthouse score calculator, if I substract 5,000 ms from Largest Contentful Paint, the performance score climbs to 99, which puts it in line with the new performance scores for the other Kitchen Sink pages.
On the flip side, even though this PR excludes images.html from Lighthouse, it actually adds more Kitchen Sink pages to the Lighthouse audit because previously only 5 pages were being checked because the default for maxAutodiscoverUrls is 5.
How I checked this PR