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.

Sign up for GitHub

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

Jump to bottom

perf(core): move scripts to document head + defer #8081

Merged
merged 10 commits into from
Oct 13, 2022
Merged

perf(core): move scripts to document head + defer #8081

merged 10 commits into from
Oct 13, 2022

Conversation

sanjaiyan-dev
Copy link
Contributor

@sanjaiyan-dev sanjaiyan-dev commented Sep 11, 2022 •
edited
Loading

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Sep 11, 2022
@netlify
Copy link

netlify bot commented Sep 11, 2022 •
edited
Loading

✅ [V2]

Name Link
🔨 Latest commit 4e33181
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/6348139f3b304500094af750
😎 Deploy Preview https://deploy-preview-8081--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@sanjaiyan-dev sanjaiyan-dev changed the title Optimizing web load speed Optimizing web load speed 🚀 Sep 11, 2022
@github-actions
Copy link

github-actions bot commented Sep 11, 2022 •
edited
Loading

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 64 🟢 98 🟢 100 🟢 100 🟠 80 Report
/docs/installation 🟠 73 🟢 100 🟢 100 🟢 100 🟢 90 Report

@sanjaiyan-dev
Copy link
Contributor Author

Done 🙌

@Josh-Cena
Copy link
Collaborator

Could you explain your changes? I don't see a potential perf difference before and after.

@sanjaiyan-dev
Copy link
Contributor Author

Could you explain your changes? I don't see a potential perf difference before and after.

Hi,

I thought by preloading CSS styles (recommend by Google) for faster loading of critical assets and making JavaScript fetch request before the body tag and make it parallel with HTML parsing would improve performance.

Ref-: https://web.dev/render-blocking-resources

Additional note -: Earlier Next.js also preloaded and then executed the code at end of the body I think after version 11.0.0 they made fetch request top of body using defer attribute which have better browser support than link:preload
Even they preload critical asset like CSS using link:preload for better FCP.

Screenshot_2022-09-11-09-37-43-341_com android chrome

Extremely sorry my thought might misleading 🥺

@Josh-Cena
Copy link
Collaborator

Leaving it to @slorber for a decision...

@Josh-Cena Josh-Cena changed the title Optimizing web load speed 🚀 perf(core): preload stylesheets and defer scripts Sep 11, 2022
@Josh-Cena Josh-Cena added the domain: performance Related to bundle size or perf optimization label Sep 11, 2022
@sanjaiyan-dev
Copy link
Contributor Author

Browser Support -:

<link rel="preload" />
Screenshot_2022-09-23-12-37-12-744_com.android.chrome.png

<script defer />
Screenshot_2022-09-23-12-36-40-735_com.android.chrome.png

@sanjaiyan-dev
Copy link
Contributor Author

Leaving it to @slorber for a decision...

Regarding preload of stylesheet -:
https://web.dev/preload-critical-assets/#preloading-css-files

slorber
slorber reviewed Sep 28, 2022
View reviewed changes
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please back your changes with more external links. Show me how other frameworks like Next.js, Remix and Gatsby are using defer (or not) for the script hydrating React.

I don't believe these changes improve anything, yet present a little risk

packages/docusaurus/src/webpack/templates/ssr.html.template.ts Outdated Show resolved Hide resolved
packages/docusaurus/src/webpack/templates/ssr.html.template.ts Show resolved Hide resolved
@sanjaiyan-dev
Copy link
Contributor Author

Please back your changes with more external links. Show me how other frameworks like Next.js, Remix and Gatsby are using defer (or not) for the script hydrating React.

I don't believe these changes improve anything, yet present a little risk

Hi, this is how next js generate html -:
Screenshot_2022-09-29-13-06-08-708_com android chrome-01

@slorber
Copy link
Collaborator

slorber commented Sep 29, 2022

I want to see the code outputing such html, and the PR that lead to switching from end-of-body scripts to head deferred scripts: this way we can understand their motivations to do this change and understand if we should follow the same path.

Yes, I expect you to do research because otherwise I'll have to do it myself 😅

@sanjaiyan-dev
Copy link
Contributor Author

I want to see the code outputing such html, and the PR that lead to switching from end-of-body scripts to head deferred scripts: this way we can understand their motivations to do this change and understand if we should follow the same path.

Yes, I expect you to do research because otherwise I'll have to do it myself 😅

Hi, by default Next js outputs this HTML (create-next-app)

@sanjaiyan-dev
Copy link
Contributor Author

I want to see the code outputing such html, and the PR that lead to switching from end-of-body scripts to head deferred scripts: this way we can understand their motivations to do this change and understand if we should follow the same path.

Yes, I expect you to do research because otherwise I'll have to do it myself 😅

@slorber

Related discussion -: vercel/next.js#24938

@slorber
Copy link
Collaborator

slorber commented Sep 30, 2022

Thanks, that's a good ref ;) will read that later

@slorber slorber marked this pull request as draft September 30, 2022 14:01
@sanjaiyan-dev
Copy link
Contributor Author

Thanks, that's a good ref ;) will read that later

Ok 💪🙌

@sanjaiyan-dev
Copy link
Contributor Author

Another useful link -: https://web.dev/render-blocking-resources

@sanjaiyan-dev sanjaiyan-dev requested review from slorber and removed request for lex111 and Josh-Cena October 1, 2022 17:52
@sanjaiyan-dev sanjaiyan-dev marked this pull request as ready for review October 7, 2022 10:47
@slorber
Copy link
Collaborator

slorber commented Oct 13, 2022

How I interpret this is that @janicklas-ralph says it's better to use this for critical JS resources:

<html>
  <head>
    <script defer src="./react-app.js" />
  </head>
  <body>
    <div id="react-app" />
  </body>
</html>

Instead of:

<html>
  <head>
    <link rel="preload" href="./react-app.js" />
  </head>
  <body>
    <div id="react-app" />
    <script async src="./react-app.js" />
  </body>
</html>

That makes sense to me, so I'll merge this change without backporting it so that we can see if any issue happens on our own website. It will be in 3.0 if proven successful.

Now you also suggested to do:

<head>
+ <link rel="preload" href="/assets/css/docusaurus.f54f9970.css" as="style">
  <thingsInBetween/>
  <link rel="stylesheet" href="/assets/css/docusaurus.f54f9970.css">
</head>

That doesn't seem useful to me: the critical CSS is already loaded quite eagerly in the header and discovered by browsers relatively early. I doubt <thingsInBetween> will lead to any significant delay in the loading of this blocking/critical CSS file.

I'll revert this preload change for now but we'll see later how to optimize CSS loading, because this critical CSS file is too large in the first place

@slorber slorber added the pr: performance This PR does not add a new behavior, but existing behaviors will be more memory- / time-efficient. label Oct 13, 2022
@slorber slorber changed the title perf(core): preload stylesheets and defer scripts perf(core): move scripts to document head + defer Oct 13, 2022
@slorber slorber merged commit 80064b2 into facebook:main Oct 13, 2022
@slorber
Copy link
Collaborator

slorber commented Oct 13, 2022

The change looks good to me, however, I'm not sure why Chrome devtools report the loading priority as "low" instead of "high" (it was "high" previously)

Is this expected @janicklas-ralph ?

Even the Next.js site loads all scripts as low priority 🤷‍♂️

CleanShot 2022-10-13 at 15 59 45@2x

@sanjaiyan-dev sanjaiyan-dev mentioned this pull request Oct 14, 2022
Closed
@sanjaiyan-dev
Copy link
Contributor Author

The change looks good to me, however, I'm not sure why Chrome devtools report the loading priority as "low" instead of "high" (it was "high" previously)

Is this expected @janicklas-ralph ?

Even the Next.js site loads all scripts as low priority 🤷‍♂️

CleanShot 2022-10-13 at 15 59 45@2x

Hi,
What about using fetchpriority="high" prop to priotize the first party scripts ?
https://web.dev/priority-hints/

Sorry if I am wrong

@sanjaiyan-dev sanjaiyan-dev deleted the sanjaiyan-css-preload branch June 23, 2023 07:44
This was referenced Oct 19, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slorber slorber slorber left review comments

Assignees
No one assigned
Labels
CLA Signed Signed Facebook CLA domain: performance Related to bundle size or perf optimization pr: performance This PR does not add a new behavior, but existing behaviors will be more memory- / time-efficient.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sanjaiyan-dev @Josh-Cena @slorber @facebook-github-bot