Skip to content

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

Closed
Josh-Cena wants to merge 8 commits into
mainfrom
jc/react-loadable
Closed

refactor: migrate from react-loadable to @react-loadable/revised#6581
Josh-Cena wants to merge 8 commits into
mainfrom
jc/react-loadable

Conversation

@Josh-Cena

Copy link
Copy Markdown
Collaborator

Motivation

Continuation of #6306. Close #3841

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

TODO

Josh-Cena and others added 5 commits February 2, 2022 15:36
I forgot to do this before.

Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
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 <24852454+seyoon20087@users.noreply.github.com>
Signed-off-by: Shinwon Elizabeth Yoon <24852454+seyoon20087@users.noreply.github.com>
@Josh-Cena Josh-Cena added the pr: maintenance This PR does not produce any behavior differences to end users when upgrading. label Feb 2, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Feb 2, 2022
@Josh-Cena

Copy link
Copy Markdown
Collaborator Author

Not sure if it even works... There are API changes between the revised version and the original version, and we have an addon that I have no idea if is impacted by this migration.

@Josh-Cena
Josh-Cena marked this pull request as draft February 2, 2022 07:47
@seyoon20087

seyoon20087 commented Feb 2, 2022

Copy link
Copy Markdown
Contributor

Try modifying these lines:

import React from 'react';
import type {LoadingComponentProps} from 'react-loadable';
export default function Loading({
error,
retry,
pastDelay,
}: LoadingComponentProps): JSX.Element | null {

To this:

diff --git a/packages/docusaurus/src/client/theme-fallback/Loading/index.tsx b/packages/docusaurus/src/client/theme-fallback/Loading/index.tsx
index 761c9c3a7..97cbac153 100644
--- a/packages/docusaurus/src/client/theme-fallback/Loading/index.tsx
+++ b/packages/docusaurus/src/client/theme-fallback/Loading/index.tsx
@@ -5,14 +5,17 @@
  * LICENSE file in the root directory of this source tree.
  */
 
+/* eslint-disable @typescript-eslint/ban-ts-comment */
+/* eslint-disable @typescript-eslint/explicit-module-boundary-types */
+
 import React from 'react';
-import type {LoadingComponentProps} from 'react-loadable';
 
 export default function Loading({
-  error,
-  retry,
+  // @ts-ignore
+  error, // @ts-ignore
+  retry, // @ts-ignore
   pastDelay,
-}: LoadingComponentProps): JSX.Element | null {
+}): JSX.Element | null {
   if (error) {
     return (
       <div

This works for me, but let me know if something needs to be improved

@Josh-Cena

Copy link
Copy Markdown
Collaborator Author

@seyoon20087 Adding ts-ignore will surely make things "work" 😄 I will try to make them actually "work" or figure out if this setup will work at all

@Josh-Cena

Copy link
Copy Markdown
Collaborator Author

@react-loadable/revised looks extremely low-quality (especially with types). I guess I would just migrate to loadable-components instead.

@Josh-Cena Josh-Cena closed this Mar 30, 2022
@Josh-Cena
Josh-Cena deleted the jc/react-loadable branch March 30, 2022 14:45
@netlify

netlify Bot commented Mar 30, 2022

Copy link
Copy Markdown

[V2]

Name Link
🔨 Latest commit 044001a
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/62446c81a70404000836edb6
😎 Deploy Preview https://deploy-preview-6581--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.

@github-actions

github-actions Bot commented Mar 30, 2022

Copy link
Copy Markdown

⚡️ Lighthouse report for the changes in this PR:

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

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

@github-actions

github-actions Bot commented Mar 30, 2022

Copy link
Copy Markdown

Size Change: +3.79 kB (0%)

Total Size: 807 kB

Filename Size Change
website/build/assets/js/main.********.js 614 kB +3.69 kB (+1%)
website/build/index.html 38.5 kB +90 B (0%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 49.9 kB
website/build/assets/css/styles.********.css 105 kB

compressed-size-action

@Josh-Cena
Josh-Cena restored the jc/react-loadable branch March 31, 2022 01:28
@Josh-Cena Josh-Cena reopened this Mar 31, 2022
@Josh-Cena Josh-Cena closed this Mar 31, 2022
@Josh-Cena
Josh-Cena deleted the jc/react-loadable branch March 31, 2022 01:34
@slorber

slorber commented Apr 6, 2022

Copy link
Copy Markdown
Collaborator

@Josh-Cena do we even still need a lib with React 18?

I'd like to figure out how to remove this.

React.lazy() now has support for Server-Side-Rendering

The v17 warning + suggestion to use loadable components was removed, so I guess our setup could be simplified 🤷‍♂️

image

The doc is not so clear about the behavior of React.lazy() with server-side-rendering behavior though, but I guess it will not show suspense placeholders and just load appropriately the lazy components? Will try to get answers

@Josh-Cena

Copy link
Copy Markdown
Collaborator Author

React.lazy doesn't work with SSG. See https://loadable-components.com/docs/loadable-vs-react-lazy/

@slorber

slorber commented Apr 6, 2022

Copy link
Copy Markdown
Collaborator

React.lazy did not work with SSG, but in React 18 it does (loadable components doc has probably not been updated since)

Confirmed by React itself :D https://twitter.com/reactjs/status/1511755578716028928

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Signed Facebook CLA pr: maintenance This PR does not produce any behavior differences to end users when upgrading.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate from react-loadable to loadable-components (or other alternatives)

4 participants