-
Notifications
You must be signed in to change notification settings - Fork 124
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
Defer scripts in page template #2492
Defer scripts in page template #2492
Conversation
Tested on google chrome on windows 10 💯💯 |
cb84513
to
69ec838
Compare
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.
do mark this as not draft and draft a PR message. you can also test async, but it seems theoretically it will be worse than defer due to potential layout shifts
my tests on mac firefox show smaller differences, but overall looks positive
No change (regular 2G):
~Finish: 1.28.5 min
~DOMContentLoaded: 1.05 min
~load: 1.27
With change (regular 2G):
~Finish: 1.21 min
~DOMContentLoaded: 59.92 s
~load: 1.22
Without change (regular 3G)
~Finish: 25.45 s
~DOMContentLoaded: 20.52 s
~load: 25.46s
With change (regular) 3G
~Finish: 25.51 s
~DOMContentLoaded: 19.36 s
~load: 23. s
Thanks for the reviews @yucheng11122017 @kaixin-hc !
Yup, I have tried using |
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.
LGTM! Thank you for the work @LamJiuFong
@kaixin-hc Each PR must have a SEMVER impact label, please remember to label the PR properly. |
This reverts commit 8ad1b0c.
What is the purpose of this pull request?
Improve performance
Fixes Load non critical resources last #1670
Overview of changes:
Add
defer
attribute to the<script>
s in page templateAnything you'd like to highlight/discuss:
defer
vs moving the script tags to the bottom:The effect of the
defer
attribute is similar to placing a conventional <script> reference at the bottom of the HTML before the closing tag, however the advantage of defer is that <script>s can be placed in the to be discovered and start loading sooner than resources found lower on the page.defer
vsasync
:async
executes the scripts once they have been fully loaded. It interrupts the rendering process and might cause some layout shiftsdefer
executes after all content has been rendered.Testing instructions:
Follow the instructions here: #1687
Things to add on:
Instead of testing on the landing page, we can test on "larger" pages (eg. UG, DG), the difference is more significant when testing on larger pages.
The PR above only tests the effect of moving the scripts to the bottom
On top of that, we can test the effect of
defer
( as shown in this pr )We can also test the effect of
async
- Simply change defer to asyncExpected results:
For all three cases, the time taken for the content to be loaded are around the same
However,
async
will cause some layout shifts when loading. (try with UserGuide/GettingStarted)Would be very grateful if anyone could help test on his/her laptop
Proposed commit message: (wrap lines at 72 characters)
Add defer attribute to scripts in page template
Checklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major
,r.Minor
,r.Patch
.Breaking change release note preparation (if applicable):