Use React Suspense when rendering async modals (#28386)

* Use React Suspense when rendering async modals Signed-off-by: Michael Telatynski <[email protected]> * Fix test Signed-off-by: Michael Telatynski <[email protected]> * Improve coverage Signed-off-by: Michael Telatynski <[email protected]> * Improve coverage Signed-off-by: Michael Telatynski <[email protected]> * Improve coverage Signed-off-by: Michael Telatynski <[email protected]> * Update src/Modal.tsx --------- Signed-off-by: Michael Telatynski <[email protected]>
element-hq · Nov 12, 2024 · 27a43e8 · 27a43e8
1 parent 9b5d086
commit 27a43e8
Show file tree

Hide file tree

Showing 17 changed files with 306 additions and 158 deletions.
diff --git a/src/AddThreepid.ts b/src/AddThreepid.ts
@@ -10,13 +10,13 @@ Please see LICENSE files in the repository root for full details.
 
 import {
     IAddThreePidOnlyBody,
-    IAuthData,
     IRequestMsisdnTokenResponse,
     IRequestTokenResponse,
     MatrixClient,
     MatrixError,
     HTTPError,
     IThreepid,
+    UIAResponse,
 } from "matrix-js-sdk/src/matrix";
 
 import Modal from "./Modal";
@@ -179,7 +179,9 @@ export default class AddThreepid {
      * with a "message" property which contains a human-readable message detailing why
      * the request failed.
      */
-    public async checkEmailLinkClicked(): Promise<[success?: boolean, result?: IAuthData | Error | null]> {
+    public async checkEmailLinkClicked(): Promise<
+        [success?: boolean, result?: UIAResponse<IAddThreePidOnlyBody> | Error | null]
+    > {
         try {
             if (this.bind) {
                 const authClient = new IdentityAuthClient();
@@ -220,7 +222,7 @@ export default class AddThreepid {
                             continueKind: "primary",
                         },
                     };
-                    const { finished } = Modal.createDialog(InteractiveAuthDialog<{}>, {
+                    const { finished } = Modal.createDialog(InteractiveAuthDialog<IAddThreePidOnlyBody>, {
                         title: _t("settings|general|add_email_dialog_title"),
                         matrixClient: this.matrixClient,
                         authData: err.data,
@@ -263,7 +265,9 @@ export default class AddThreepid {
      * with a "message" property which contains a human-readable message detailing why
      * the request failed.
      */
-    public async haveMsisdnToken(msisdnToken: string): Promise<[success?: boolean, result?: IAuthData | Error | null]> {
+    public async haveMsisdnToken(
+        msisdnToken: string,
+    ): Promise<[success?: boolean, result?: UIAResponse<IAddThreePidOnlyBody> | Error | null]> {
         const authClient = new IdentityAuthClient();
 
         if (this.submitUrl) {
@@ -319,7 +323,7 @@ export default class AddThreepid {
                         continueKind: "primary",
                     },
                 };
-                const { finished } = Modal.createDialog(InteractiveAuthDialog<{}>, {
+                const { finished } = Modal.createDialog(InteractiveAuthDialog<IAddThreePidOnlyBody>, {
                     title: _t("settings|general|add_msisdn_dialog_title"),
                     matrixClient: this.matrixClient,
                     authData: err.data,

diff --git a/src/AsyncWrapper.tsx b/src/AsyncWrapper.tsx
@@ -6,24 +6,19 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only
 Please see LICENSE files in the repository root for full details.
 */
 
-import React, { ComponentType, PropsWithChildren } from "react";
-import { logger } from "matrix-js-sdk/src/logger";
+import React, { ReactNode, Suspense } from "react";
 
 import { _t } from "./languageHandler";
 import BaseDialog from "./components/views/dialogs/BaseDialog";
 import DialogButtons from "./components/views/elements/DialogButtons";
 import Spinner from "./components/views/elements/Spinner";
 
-type AsyncImport<T> = { default: T };
-
 interface IProps {
-    // A promise which resolves with the real component
-    prom: Promise<ComponentType<any> | AsyncImport<ComponentType<any>>>;
     onFinished(): void;
+    children: ReactNode;
 }
 
 interface IState {
-    component?: ComponentType<PropsWithChildren<any>>;
     error?: Error;
 }
 
@@ -32,56 +27,26 @@ interface IState {
  * spinner until the real component loads.
  */
 export default class AsyncWrapper extends React.Component<IProps, IState> {
-    private unmounted = false;
-
-    public state: IState = {};
-
-    public componentDidMount(): void {
-        this.unmounted = false;
-        this.props.prom
-            .then((result) => {
-                if (this.unmounted) return;
-
-                // Take the 'default' member if it's there, then we support
-                // passing in just an import()ed module, since ES6 async import
-                // always returns a module *namespace*.
-                const component = (result as AsyncImport<ComponentType>).default
-                    ? (result as AsyncImport<ComponentType>).default
-                    : (result as ComponentType);
-                this.setState({ component });
-            })
-            .catch((e) => {
-                logger.warn("AsyncWrapper promise failed", e);
-                this.setState({ error: e });
-            });
+    public static getDerivedStateFromError(error: Error): IState {
+        return { error };
     }
 
-    public componentWillUnmount(): void {
-        this.unmounted = true;
-    }
-
-    private onWrapperCancelClick = (): void => {
-        this.props.onFinished();
-    };
+    public state: IState = {};
 
     public render(): React.ReactNode {
-        if (this.state.component) {
-            const Component = this.state.component;
-            return <Component {...this.props} />;
-        } else if (this.state.error) {
+        if (this.state.error) {
             return (
                 <BaseDialog onFinished={this.props.onFinished} title={_t("common|error")}>
                     {_t("failed_load_async_component")}
                     <DialogButtons
                         primaryButton={_t("action|dismiss")}
-                        onPrimaryButtonClick={this.onWrapperCancelClick}
+                        onPrimaryButtonClick={this.props.onFinished}
                         hasCancel={false}
                     />
                 </BaseDialog>
             );
-        } else {
-            // show a spinner until the component is loaded.
-            return <Spinner />;
         }
+
+        return <Suspense fallback={<Spinner />}>{this.props.children}</Suspense>;
     }
 }
diff --git a/src/Modal.tsx b/src/Modal.tsx
@@ -136,32 +136,6 @@ export class ModalManager extends TypedEventEmitter<ModalManagerEvent, HandlerMa
         return !!this.priorityModal || !!this.staticModal || this.modals.length > 0;
     }
 
-    public createDialog<C extends ComponentType>(
-        Element: C,
-        props?: ComponentProps<C>,
-        className?: string,
-        isPriorityModal = false,
-        isStaticModal = false,
-        options: IOptions<C> = {},
-    ): IHandle<C> {
-        return this.createDialogAsync<C>(
-            Promise.resolve(Element),
-            props,
-            className,
-            isPriorityModal,
-            isStaticModal,
-            options,
-        );
-    }
-
-    public appendDialog<C extends ComponentType>(
-        Element: C,
-        props?: ComponentProps<C>,
-        className?: string,
-    ): IHandle<C> {
-        return this.appendDialogAsync<C>(Promise.resolve(Element), props, className);
-    }
-
     /**
      * DEPRECATED.
      * This is used only for tests. They should be using forceCloseAllModals but that
@@ -196,8 +170,11 @@ export class ModalManager extends TypedEventEmitter<ModalManagerEvent, HandlerMa
         this.reRender();
     }
 
+    /**
+     * @typeParam C - the component type
+     */
     private buildModal<C extends ComponentType>(
-        prom: Promise<C>,
+        Component: C,
         props?: ComponentProps<C>,
         className?: string,
         options?: IOptions<C>,
@@ -222,9 +199,12 @@ export class ModalManager extends TypedEventEmitter<ModalManagerEvent, HandlerMa
         // otherwise we'll get confused.
         const modalCount = this.counter++;
 
-        // FIXME: If a dialog uses getDefaultProps it clobbers the onFinished
-        // property set here so you can't close the dialog from a button click!
-        modal.elem = <AsyncWrapper key={modalCount} prom={prom} {...props} onFinished={closeDialog} />;
+        // Typescript doesn't like us passing props as any here, but we know that they are well typed due to the rigorous generics.
+        modal.elem = (
+            <AsyncWrapper key={modalCount} onFinished={closeDialog}>
+                <Component {...(props as any)} onFinished={closeDialog} />
+            </AsyncWrapper>
+        );
         modal.close = closeDialog;
 
         return { modal, closeDialog, onFinishedProm };
@@ -291,37 +271,38 @@ export class ModalManager extends TypedEventEmitter<ModalManagerEvent, HandlerMa
      *       require(['<module>'], cb);
      *   }
      *
-     * @param {Promise} prom   a promise which resolves with a React component
-     *   which will be displayed as the modal view.
+     * @param component The component to render as a dialog. This component must accept an `onFinished` prop function as
+     *                  per the type {@link ComponentType}. If loading a component with esoteric dependencies consider
+     *                  using React.lazy to async load the component.
+     *                  e.g. `lazy(() => import('./MyComponent'))`
      *
-     * @param {Object} props   properties to pass to the displayed
-     *    component. (We will also pass an 'onFinished' property.)
+     * @param props properties to pass to the displayed component. (We will also pass an 'onFinished' property.)
      *
-     * @param {String} className   CSS class to apply to the modal wrapper
+     * @param className CSS class to apply to the modal wrapper
      *
-     * @param {boolean} isPriorityModal if true, this modal will be displayed regardless
+     * @param isPriorityModal if true, this modal will be displayed regardless
      *                                  of other modals that are currently in the stack.
      *                                  Also, when closed, all modals will be removed
      *                                  from the stack.
-     * @param {boolean} isStaticModal  if true, this modal will be displayed under other
+     * @param isStaticModal if true, this modal will be displayed under other
      *                                 modals in the stack. When closed, all modals will
      *                                 also be removed from the stack. This is not compatible
      *                                 with being a priority modal. Only one modal can be
      *                                 static at a time.
-     * @param {Object} options? extra options for the dialog
-     * @param {onBeforeClose} options.onBeforeClose a callback to decide whether to close the dialog
-     * @returns {object} Object with 'close' parameter being a function that will close the dialog
+     * @param options? extra options for the dialog
+     * @param options.onBeforeClose a callback to decide whether to close the dialog
+     * @returns Object with 'close' parameter being a function that will close the dialog
      */
-    public createDialogAsync<C extends ComponentType>(
-        prom: Promise<C>,
+    public createDialog<C extends ComponentType>(
+        component: C,
         props?: ComponentProps<C>,
         className?: string,
         isPriorityModal = false,
         isStaticModal = false,
         options: IOptions<C> = {},
     ): IHandle<C> {
         const beforeModal = this.getCurrentModal();
-        const { modal, closeDialog, onFinishedProm } = this.buildModal<C>(prom, props, className, options);
+        const { modal, closeDialog, onFinishedProm } = this.buildModal<C>(component, props, className, options);
         if (isPriorityModal) {
             // XXX: This is destructive
             this.priorityModal = modal;
@@ -341,13 +322,13 @@ export class ModalManager extends TypedEventEmitter<ModalManagerEvent, HandlerMa
         };
     }
 
-    private appendDialogAsync<C extends ComponentType>(
-        prom: Promise<C>,
+    public appendDialog<C extends ComponentType>(
+        component: C,
         props?: ComponentProps<C>,
         className?: string,
     ): IHandle<C> {
         const beforeModal = this.getCurrentModal();
-        const { modal, closeDialog, onFinishedProm } = this.buildModal<C>(prom, props, className, {});
+        const { modal, closeDialog, onFinishedProm } = this.buildModal<C>(component, props, className, {});
 
         this.modals.push(modal);
 

diff --git a/src/SecurityManager.ts b/src/SecurityManager.ts
@@ -6,11 +6,11 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only
 Please see LICENSE files in the repository root for full details.
 */
 
+import { lazy } from "react";
 import { ICryptoCallbacks, SecretStorage } from "matrix-js-sdk/src/matrix";
 import { deriveRecoveryKeyFromPassphrase, decodeRecoveryKey } from "matrix-js-sdk/src/crypto-api";
 import { logger } from "matrix-js-sdk/src/logger";
 
-import type CreateSecretStorageDialog from "./async-components/views/dialogs/security/CreateSecretStorageDialog";
 import Modal from "./Modal";
 import { MatrixClientPeg } from "./MatrixClientPeg";
 import { _t } from "./languageHandler";
@@ -232,10 +232,8 @@ async function doAccessSecretStorage(func: () => Promise<void>, forceReset: bool
         if (createNew) {
             // This dialog calls bootstrap itself after guiding the user through
             // passphrase creation.
-            const { finished } = Modal.createDialogAsync(
-                import("./async-components/views/dialogs/security/CreateSecretStorageDialog") as unknown as Promise<
-                    typeof CreateSecretStorageDialog
-                >,
+            const { finished } = Modal.createDialog(
+                lazy(() => import("./async-components/views/dialogs/security/CreateSecretStorageDialog")),
                 {
                     forceReset,
                 },

diff --git a/src/async-components/views/dialogs/security/NewRecoveryMethodDialog.tsx b/src/async-components/views/dialogs/security/NewRecoveryMethodDialog.tsx
@@ -28,7 +28,7 @@ interface NewRecoveryMethodDialogProps {
     onFinished(): void;
 }
 
-// Export as default instead of a named export so that it can be dynamically imported with `Modal.createDialogAsync`
+// Export as default instead of a named export so that it can be dynamically imported with React lazy
 
 /**
  * Dialog to inform the user that a new recovery method has been detected.

diff --git a/src/async-components/views/dialogs/security/RecoveryMethodRemovedDialog.tsx b/src/async-components/views/dialogs/security/RecoveryMethodRemovedDialog.tsx
@@ -7,11 +7,11 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only
 Please see LICENSE files in the repository root for full details.
 */
 
-import React from "react";
+import React, { lazy } from "react";
 
 import dis from "../../../../dispatcher/dispatcher";
 import { _t } from "../../../../languageHandler";
-import Modal, { ComponentType } from "../../../../Modal";
+import Modal from "../../../../Modal";
 import { Action } from "../../../../dispatcher/actions";
 import BaseDialog from "../../../../components/views/dialogs/BaseDialog";
 import DialogButtons from "../../../../components/views/elements/DialogButtons";
@@ -28,8 +28,8 @@ export default class RecoveryMethodRemovedDialog extends React.PureComponent<IPr
 
     private onSetupClick = (): void => {
         this.props.onFinished();
-        Modal.createDialogAsync(
-            import("./CreateKeyBackupDialog") as unknown as Promise<ComponentType>,
+        Modal.createDialog(
+            lazy(() => import("./CreateKeyBackupDialog")),
             undefined,
             undefined,
             /* priority = */ false,

diff --git a/src/components/structures/MatrixChat.tsx b/src/components/structures/MatrixChat.tsx
@@ -6,7 +6,7 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only
 Please see LICENSE files in the repository root for full details.
 */
 
-import React, { createRef } from "react";
+import React, { createRef, lazy } from "react";
 import {
     ClientEvent,
     createClient,
@@ -28,8 +28,6 @@ import { TooltipProvider } from "@vector-im/compound-web";
 // what-input helps improve keyboard accessibility
 import "what-input";
 
-import type NewRecoveryMethodDialog from "../../async-components/views/dialogs/security/NewRecoveryMethodDialog";
-import type RecoveryMethodRemovedDialog from "../../async-components/views/dialogs/security/RecoveryMethodRemovedDialog";
 import PosthogTrackers from "../../PosthogTrackers";
 import { DecryptionFailureTracker } from "../../DecryptionFailureTracker";
 import { IMatrixClientCreds, MatrixClientPeg } from "../../MatrixClientPeg";
@@ -1649,16 +1647,12 @@ export default class MatrixChat extends React.PureComponent<IProps, IState> {
             }
 
             if (haveNewVersion) {
-                Modal.createDialogAsync(
-                    import(
-                        "../../async-components/views/dialogs/security/NewRecoveryMethodDialog"
-                    ) as unknown as Promise<typeof NewRecoveryMethodDialog>,
+                Modal.createDialog(
+                    lazy(() => import("../../async-components/views/dialogs/security/NewRecoveryMethodDialog")),
                 );
             } else {
-                Modal.createDialogAsync(
-                    import(
-                        "../../async-components/views/dialogs/security/RecoveryMethodRemovedDialog"
-                    ) as unknown as Promise<typeof RecoveryMethodRemovedDialog>,
+                Modal.createDialog(
+                    lazy(() => import("../../async-components/views/dialogs/security/RecoveryMethodRemovedDialog")),
                 );
             }
         });