feat(core): use react-helmet-async #6306

seyoon20087 · 2022-01-10T09:51:38Z

Motivation

Utilize react-helmet-async on @docusaurus/Head

See the description in each commit for a reason why I did this.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Replace react-helmet with react-helmet-async in packages/docusaurus
Include HelmetProvider in both client and server

Related PRs

N/A

Even though Strict Mode is not required a WARNING icon now displays on all components that do not use React.StrictMode on React DevTools extension. Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>

react-helmet is NOT thread safe, as explained in https://open.nytimes.com/the-future-of-meta-tag-management-for-modern-react-development-ec26a7dc9183#fdc2 Therefore, it's better if react-helmet-async is utilized instead of react-helmet. Even though react-helmet-async is being utilized, most users will not require any code changes to @docusaurus/Head since it uses the same API as react-helmet. Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>

I forgot to do this before. Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>

netlify · 2022-01-10T09:56:57Z

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 4f2ddd1

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61fa26ba7ec05600089c56dc

😎 Browse the preview: https://deploy-preview-6306--docusaurus-2.netlify.app

github-actions · 2022-01-10T09:57:28Z

⚡️ Lighthouse report for the changes in this PR:

Category	Score
🟠 Performance	66
🟢 Accessibility	100
🟢 Best practices	100
🟢 SEO	100
🟢 PWA	92

Lighthouse ran on https://deploy-preview-6306--docusaurus-2.netlify.app/

Josh-Cena · 2022-01-10T10:02:08Z

Great, it makes sense to me 👍

Strict mode may cause surprising useEffect magic to some users, but I guess it's mostly fine

slorber · 2022-01-12T15:59:41Z

Thanks 👍 Definitively something I wanted to do.

Will review this more carefully and see potential side-effects for users later

slorber · 2022-01-12T16:02:30Z

packages/docusaurus/src/client/clientEntry.tsx

-      </BrowserRouter>,
+      <React.StrictMode>
+        <BrowserRouter>
+          <HelmetProvider>


for consistency between client/server, maybe apply the new provider above the router in both cases?

slorber · 2022-01-12T16:04:12Z

packages/docusaurus/src/client/serverEntry.tsx

@@ -83,19 +83,23 @@ async function doRender(locals: Locals & {path: string}) {
  const modules = new Set<string>();
  const context = {};

+  const helmetContext = {};


maybe type this instead and use ! or a if(!context.helmet) guard

This how the HelmetProvider is supposed to work. Type for HelmetProvider.props.context is {} although I suspect that's just a bad typing and they really just mean Record<string, never>

slorber · 2022-01-12T16:08:19Z

packages/docusaurus/src/client/serverEntry.tsx

-          <App />
-        </ProvideLinksCollector>
-      </StaticRouter>
+      <HelmetProvider context={helmetContext}>


probably worth applying strict mode here too for security, even if I doubt it will be very useful (only enabled in prod) we may enable some SSR in development mode in the future.

See also https://twitter.com/sebastienlorber/status/1481295361016745984

slorber

This works in prod, but produces a lot of warnings in dev due to react-loadable

Unless we update our react-loadable fork (https://github.com/slorber/react-loadable?organization=slorber&organization=slorber), it's not possible to enable strict mode above the route components, so at most it can be just above the layout

React-loadable is using the very old React context feature: https://github.com/slorber/react-loadable/blob/master/src/index.js#L278 and lots of UNSAFE lifecycles

Josh-Cena · 2022-01-22T06:44:23Z

Hey, you are certainly doing something wrong with your branch😅 Just try rebase main

Josh-Cena · 2022-01-22T06:57:29Z

@seyoon20087 Can you handle this? Maybe you want to sit back and let me fix your branch? I'm getting lots of notifications of these commits.

seyoon20087 · 2022-01-22T06:58:38Z

Sure you may fix them

Josh-Cena · 2022-01-22T07:11:15Z

So, we for sure don't want to maintain these dependencies in this repo. FYI, we have just acquired an organization where we can host our forked dependencies. If there are incompatible dependencies (e.g. react-loadable) we will fix it in our forked versions. We have already forked react-loadable, so there's nothing you need to do.

seyoon20087 · 2022-01-22T07:14:14Z

The forked react-loadable package had some warnings logged in the console, as described here...

Josh-Cena · 2022-01-22T07:16:17Z

Yes, we will continue to improve on our fork or inspect other alternatives, but for now there isn't a decision yet. react-loadble is a bit complicated, because we may not use this dependency at all in the near future. But for sure, we would not want to self-host a script file like you currently do.

Both unforked react-loadable and @docusaurus/react-loadable uses legacy React APIs. However, @react-loadable/revised (https://github.com/react-loadable/revised) is actively maintained and widely used in production, thus replaced with this package. Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>

seyoon20087 · 2022-01-22T17:28:03Z

This works in prod, but produces a lot of warnings in dev due to react-loadable

Unless we update our react-loadable fork (https://github.com/slorber/react-loadable?organization=slorber&organization=slorber), it's not possible to enable strict mode above the route components, so at most it can be just above the layout

React-loadable is using the very old React context feature: https://github.com/slorber/react-loadable/blob/master/src/index.js#L278 and lots of UNSAFE lifecycles

@slorber I just fixed the issue. Could you take a look?

slorber

Didn't know about this revised fork (https://github.com/react-loadable/revised)
That looks suitable to replace our own fork (https://github.com/slorber/react-loadable)

TS problems to fix

why not using the latest version?

I'll check later to see if strict mode works

Changing the fork is a quite important change, considering the implementation is totally refactored. We'd rather have 2 separate PRs: one for the fork and one for strict mode

slorber · 2022-01-27T11:34:36Z

packages/docusaurus/package.json

@@ -44,7 +44,7 @@
    "@docusaurus/cssnano-preset": "2.0.0-beta.15",
    "@docusaurus/logger": "2.0.0-beta.15",
    "@docusaurus/mdx-loader": "2.0.0-beta.15",
-    "@docusaurus/react-loadable": "5.5.2",
+    "@docusaurus/react-loadable": "npm:@react-loadable/[email protected]",


Suggested change

"@docusaurus/react-loadable": "npm:@react-loadable/revised@0.0.26",

"@react-loadable/revised": "0.0.26",

Also why not using the latest version? 🤔

Curious why we had this package alias? To trick TS and get the correct typedefs?

which package alias? @docusaurus/react-loadable is our fork, we can't use just react-loadable

Yes I know, but we still imported react-loadable in the code. Is that so we get the correct type defs from @types/react-loadable?

Josh-Cena · 2022-01-27T11:41:20Z

My thought was that we should have had a separate PR for react-helmet-async. That way this PR would have made it into the last release instead of being blocked till now.

Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>

slorber · 2022-01-28T11:30:30Z

Can we split this PR in 3 so that easy part can be easily merged/reviewed/discussed separately?

use react-helmet-async
use @react-loadable/revised
use StrictMode

All those things are quite unrelated and some are safer than others, so lets merge these changes one at a time

seyoon20087 · 2022-01-28T11:33:08Z

Sure, of course, you may do so if you wish to

…

On Jan 28, 2022, at 8:30 PM, Sébastien Lorber ***@***.***> wrote: Can we split this PR in 3 so that easy part can be easily merged/reviewed/discussed separately? use react-helmet-async use @react-loadable/revised use StrictMode All those things are quite unrelated and some are safer than others, so lets merge these changes one at a time — Reply to this email directly, view it on GitHub <#6306 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF5TPZU5XFUMEAYMFJ2YVPDUYJ46VANCNFSM5LTGWZ5Q>. You are receiving this because you were mentioned.

Josh-Cena

We'll merge this one with only the react-helmet change. I'll cherry-pick the other changes into a new PR.

author Joshua Chen <[email protected]> 1643786935 +0800 committer Joshua Chen <[email protected]> 1643786935 +0800 gpgsig -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEE/nJvfdEj7jAI5zKOw3FFuBi9to8FAmH6Mr0ACgkQw3FFuBi9 to9wSg//WhlHmJqJenph18u1J8+W0ApnIOoSq9dck7SUjbVWbwZyUgAkgUnhZAu8 PMIl/EuIPc55jvXXQ/Vd2mtIMlHTGZA9z1I0JVZwdAag59acF/aJQAZxp6RhtWjX 6wDqc09ZRU4C8QylY6ShAFkOsewmzcCuu/tdiQbirPE3tD1qYRyZLwnru0FAzqzo V2kI6lgJMn6AVb1oqkm5lzAwxTqzet9h89z/37O0xFbBEzz3Yk26LxR8Voq2f4Tk 8XUwn1X9rRB41fGD8IkT+M23VJG2oLktcuxcSsBSTiomDtPl47ujpouBEtMTJynk wJRS/asbEM8Qu3/ayXjLFSfQhZK9ITZ3QIx9AF5gMVe2WkVAUX2voh4PYN2BSrd/ +wP/jE3Fv+cmjg6BnYeaN1xzH02QU6kwGDnvb8sw9ldnPiEWQqLqjPDPUaNTn4CH iw30Lt+WUCwtJFCKoPjQmviC6lvvqc5Neoqm+nBoPxHdW4dVHHSOXAzZUqD/TPcZ Yz5fLJi7ADj6SJL/lBxvQznFM914gmYteKxyN8QyV9qVYCtYuL5GpbK+ttEFLwMz uCaLYjDY8ZsjBqFLIXasQu49lBzLRSevc0XGME+acsh8l1cR46/kHIuV1LL5DgyT EAfRuTiMobBI+ycaelsEqgF4lW/F2Gzigjs6NmJ8/gGwHt0YjmA= =n38o -----END PGP SIGNATURE----- feat(core): use react-helmet-async (#6306) * Use React Strict Mode Even though Strict Mode is not required a WARNING icon now displays on all components that do not use React.StrictMode on React DevTools extension. Signed-off-by: Shinwon Elizabeth Yoon <[email protected]> * Utilize react-helmet-async instead of react-helmet react-helmet is NOT thread safe, as explained in https://open.nytimes.com/the-future-of-meta-tag-management-for-modern-react-development-ec26a7dc9183#fdc2 Therefore, it's better if react-helmet-async is utilized instead of react-helmet. Even though react-helmet-async is being utilized, most users will not require any code changes to @docusaurus/Head since it uses the same API as react-helmet. Signed-off-by: Shinwon Elizabeth Yoon <[email protected]> * Include HelmetProvider inside client entry I forgot to do this before. Signed-off-by: Shinwon Elizabeth Yoon <[email protected]> * format * fix TS * address reviews * Remove forked react-loadable package in favor of @react-loadable/revised Both unforked react-loadable and @docusaurus/react-loadable uses legacy React APIs. However, @react-loadable/revised (https://github.com/react-loadable/revised) is actively maintained and widely used in production, thus replaced with this package. Signed-off-by: Shinwon Elizabeth Yoon <[email protected]> * remove unused comma * Address reviews from #6306 (review) Signed-off-by: Shinwon Elizabeth Yoon <[email protected]> Co-authored-by: Joshua Chen <[email protected]> Include HelmetProvider inside client entry I forgot to do this before. Signed-off-by: Shinwon Elizabeth Yoon <[email protected]> fix TS address reviews Remove forked react-loadable package in favor of @react-loadable/revised Both unforked react-loadable and @docusaurus/react-loadable uses legacy React APIs. However, @react-loadable/revised (https://github.com/react-loadable/revised) is actively maintained and widely used in production, thus replaced with this package. Signed-off-by: Shinwon Elizabeth Yoon <[email protected]> remove unused comma

Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>

seyoon20087 added 3 commits January 10, 2022 17:38

Use React Strict Mode

bdb21ba

Even though Strict Mode is not required a WARNING icon now displays on all components that do not use React.StrictMode on React DevTools extension. Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>

Include HelmetProvider inside client entry

468c3fc

I forgot to do this before. Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>

seyoon20087 requested review from Josh-Cena, lex111 and slorber as code owners January 10, 2022 09:51

facebook-github-bot added the CLA Signed Signed Facebook CLA label Jan 10, 2022

format

e8c8204

Josh-Cena changed the title ~~Utilize React Strict Mode (on client) and react-helmet-async (on @docusaurus/Head)~~ feat: use React Strict Mode and react-helmet-async Jan 10, 2022

Josh-Cena changed the title ~~feat: use React Strict Mode and react-helmet-async~~ feat(core): use React Strict Mode and react-helmet-async Jan 10, 2022

Josh-Cena added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Jan 10, 2022

fix TS

01302f6

slorber reviewed Jan 12, 2022

View reviewed changes

address reviews

fb2c578

Josh-Cena requested a review from slorber January 14, 2022 01:09

Merge branch 'main' into use_react_strict_mode_and_react_helmet_async

731e763

slorber requested changes Jan 20, 2022

View reviewed changes

slorber marked this pull request as draft January 20, 2022 18:19

seyoon20087 marked this pull request as ready for review January 22, 2022 01:46

seyoon20087 marked this pull request as draft January 22, 2022 03:01

seyoon20087 requested a review from slorber January 22, 2022 03:01

seyoon20087 marked this pull request as ready for review January 22, 2022 17:15

Merge branch 'main' into use_react_strict_mode_and_react_helmet_async

5f77a30

seyoon20087 added 2 commits January 24, 2022 13:12

remove unused comma

449ad82

Merge branch 'main' into use_react_strict_mode_and_react_helmet_async

58693b1

slorber requested changes Jan 27, 2022

View reviewed changes

seyoon20087 changed the title ~~feat(core): use React Strict Mode and react-helmet-async~~ feat(core): use react-helmet-async on @docusaurus/Head Jan 28, 2022

Josh-Cena requested a review from slorber January 28, 2022 07:53

seyoon20087 changed the title ~~feat(core): use react-helmet-async on @docusaurus/Head~~ feat(core): use React Strict Mode and react-helmet-async Jan 28, 2022

Address reviews from #6306 (review)

11ed981

Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>

Merge branch 'main' into use_react_strict_mode_and_react_helmet_async

4f2ddd1

Josh-Cena changed the title ~~feat(core): use React Strict Mode and react-helmet-async~~ feat(core): use react-helmet-async Feb 2, 2022

Josh-Cena approved these changes Feb 2, 2022

View reviewed changes

Josh-Cena merged commit a615ab3 into facebook:main Feb 2, 2022

Josh-Cena pushed a commit that referenced this pull request Feb 2, 2022

Address reviews from #6306 (review)

7438b4f

Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>

Josh-Cena pushed a commit that referenced this pull request Feb 2, 2022

Address reviews from #6306 (review)

fef04a7

Signed-off-by: Shinwon Elizabeth Yoon <[email protected]>

Josh-Cena mentioned this pull request Feb 2, 2022

refactor: migrate from react-loadable to @react-loadable/revised #6581

Closed

Josh-Cena mentioned this pull request Feb 11, 2022

refactor(module-aliases): remove react-helmet dependency #6658

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): use react-helmet-async #6306

feat(core): use react-helmet-async #6306

seyoon20087 commented Jan 10, 2022 •

edited

Loading

netlify bot commented Jan 10, 2022 •

edited

Loading

github-actions bot commented Jan 10, 2022 •

edited

Loading

Josh-Cena commented Jan 10, 2022

slorber commented Jan 12, 2022

slorber Jan 12, 2022

slorber Jan 12, 2022

Josh-Cena Jan 13, 2022

slorber Jan 12, 2022

slorber left a comment •

edited

Loading

Josh-Cena commented Jan 22, 2022

Josh-Cena commented Jan 22, 2022 •

edited

Loading

seyoon20087 commented Jan 22, 2022

Josh-Cena commented Jan 22, 2022

seyoon20087 commented Jan 22, 2022

Josh-Cena commented Jan 22, 2022 •

edited

Loading

seyoon20087 commented Jan 22, 2022 •

edited

Loading

slorber left a comment

slorber Jan 27, 2022

Josh-Cena Jan 27, 2022

slorber Jan 28, 2022

Josh-Cena Jan 28, 2022

Josh-Cena commented Jan 27, 2022

slorber commented Jan 28, 2022 •

edited

Loading

seyoon20087 commented Jan 28, 2022 via email

Josh-Cena left a comment

	"@docusaurus/react-loadable": "npm:@react-loadable/revised@0.0.26",
	"@react-loadable/revised": "0.0.26",

feat(core): use react-helmet-async #6306

feat(core): use react-helmet-async #6306

Conversation

seyoon20087 commented Jan 10, 2022 • edited Loading

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Related PRs

netlify bot commented Jan 10, 2022 • edited Loading

github-actions bot commented Jan 10, 2022 • edited Loading

Josh-Cena commented Jan 10, 2022

slorber commented Jan 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slorber left a comment • edited Loading

Choose a reason for hiding this comment

Josh-Cena commented Jan 22, 2022

Josh-Cena commented Jan 22, 2022 • edited Loading

seyoon20087 commented Jan 22, 2022

Josh-Cena commented Jan 22, 2022

seyoon20087 commented Jan 22, 2022

Josh-Cena commented Jan 22, 2022 • edited Loading

seyoon20087 commented Jan 22, 2022 • edited Loading

slorber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josh-Cena commented Jan 27, 2022

slorber commented Jan 28, 2022 • edited Loading

seyoon20087 commented Jan 28, 2022 via email

Josh-Cena left a comment

Choose a reason for hiding this comment

seyoon20087 commented Jan 10, 2022 •

edited

Loading

netlify bot commented Jan 10, 2022 •

edited

Loading

github-actions bot commented Jan 10, 2022 •

edited

Loading

slorber left a comment •

edited

Loading

Josh-Cena commented Jan 22, 2022 •

edited

Loading

Josh-Cena commented Jan 22, 2022 •

edited

Loading

seyoon20087 commented Jan 22, 2022 •

edited

Loading

slorber commented Jan 28, 2022 •

edited

Loading