Skip to content

Commit 502cc91

Browse files
t3chguyt3chguy
authored
Switch ModalManager to the React 18 createRoot API (#28336)
* Remove boilerplate around dispatcher and settings watchers Signed-off-by: Michael Telatynski <[email protected]> * Iterate Signed-off-by: Michael Telatynski <[email protected]> * Move state update listeners from constructor to componentDidMount Signed-off-by: Michael Telatynski <[email protected]> * Iterate Signed-off-by: Michael Telatynski <[email protected]> * Switch ModalManager to the React 18 createRoot API Signed-off-by: Michael Telatynski <[email protected]> * Iterate Signed-off-by: Michael Telatynski <[email protected]> * Iterate Signed-off-by: Michael Telatynski <[email protected]> --------- Signed-off-by: Michael Telatynski <[email protected]>
1 parent 38e5eee commit 502cc91

File tree

9 files changed

+66
-60
lines changed

9 files changed

+66
-60
lines changed

‎src/Modal.tsx‎

Lines changed: 30 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,9 @@ Please see LICENSE files in the repository root for full details.
88
*/
99

1010
import React, { StrictMode } from "react";
11-
import ReactDOM from "react-dom";
11+
import { createRoot, Root } from "react-dom/client";
1212
import classNames from "classnames";
13-
import { IDeferred, defer, sleep } from "matrix-js-sdk/src/utils";
13+
import { IDeferred, defer } from "matrix-js-sdk/src/utils";
1414
import { TypedEventEmitter } from "matrix-js-sdk/src/matrix";
1515
import { Glass, TooltipProvider } from "@vector-im/compound-web";
1616

@@ -69,6 +69,16 @@ type HandlerMap = {
6969

7070
type ModalCloseReason = "backgroundClick";
7171

72+
function getOrCreateContainer(id: string): HTMLDivElement {
73+
let container = document.getElementById(id) as HTMLDivElement | null;
74+
if (!container) {
75+
container = document.createElement("div");
76+
container.id = id;
77+
document.body.appendChild(container);
78+
}
79+
return container;
80+
}
81+
7282
export class ModalManager extends TypedEventEmitter<ModalManagerEvent, HandlerMap> {
7383
private counter = 0;
7484
// The modal to prioritise over all others. If this is set, only show
@@ -83,28 +93,22 @@ export class ModalManager extends TypedEventEmitter<ModalManagerEvent, HandlerMa
8393
// Neither the static nor priority modal will be in this list.
8494
private modals: IModal<any>[] = [];
8595

86-
private static getOrCreateContainer(): HTMLElement {
87-
let container = document.getElementById(DIALOG_CONTAINER_ID);
88-
89-
if (!container) {
90-
container = document.createElement("div");
91-
container.id = DIALOG_CONTAINER_ID;
92-
document.body.appendChild(container);
96+
private static root?: Root;
97+
private static getOrCreateRoot(): Root {
98+
if (!ModalManager.root) {
99+
const container = getOrCreateContainer(DIALOG_CONTAINER_ID);
100+
ModalManager.root = createRoot(container);
93101
}
94-
95-
return container;
102+
return ModalManager.root;
96103
}
97104

98-
private static getOrCreateStaticContainer(): HTMLElement {
99-
let container = document.getElementById(STATIC_DIALOG_CONTAINER_ID);
100-
101-
if (!container) {
102-
container = document.createElement("div");
103-
container.id = STATIC_DIALOG_CONTAINER_ID;
104-
document.body.appendChild(container);
105+
private static staticRoot?: Root;
106+
private static getOrCreateStaticRoot(): Root {
107+
if (!ModalManager.staticRoot) {
108+
const container = getOrCreateContainer(STATIC_DIALOG_CONTAINER_ID);
109+
ModalManager.staticRoot = createRoot(container);
105110
}
106-
107-
return container;
111+
return ModalManager.staticRoot;
108112
}
109113

110114
public constructor() {
@@ -389,19 +393,14 @@ export class ModalManager extends TypedEventEmitter<ModalManagerEvent, HandlerMa
389393
}
390394

391395
private async reRender(): Promise<void> {
392-
// TODO: We should figure out how to remove this weird sleep. It also makes testing harder
393-
//
394-
// await next tick because sometimes ReactDOM can race with itself and cause the modal to wrongly stick around
395-
await sleep(0);
396-
397396
if (this.modals.length === 0 && !this.priorityModal && !this.staticModal) {
398397
// If there is no modal to render, make all of Element available
399398
// to screen reader users again
400399
dis.dispatch({
401400
action: "aria_unhide_main_app",
402401
});
403-
ReactDOM.unmountComponentAtNode(ModalManager.getOrCreateContainer());
404-
ReactDOM.unmountComponentAtNode(ModalManager.getOrCreateStaticContainer());
402+
ModalManager.getOrCreateRoot().render(<></>);
403+
ModalManager.getOrCreateStaticRoot().render(<></>);
405404
return;
406405
}
407406

@@ -432,10 +431,10 @@ export class ModalManager extends TypedEventEmitter<ModalManagerEvent, HandlerMa
432431
</StrictMode>
433432
);
434433

435-
ReactDOM.render(staticDialog, ModalManager.getOrCreateStaticContainer());
434+
ModalManager.getOrCreateStaticRoot().render(staticDialog);
436435
} else {
437436
// This is safe to call repeatedly if we happen to do that
438-
ReactDOM.unmountComponentAtNode(ModalManager.getOrCreateStaticContainer());
437+
ModalManager.getOrCreateStaticRoot().render(<></>);
439438
}
440439

441440
const modal = this.getCurrentModal();
@@ -461,10 +460,10 @@ export class ModalManager extends TypedEventEmitter<ModalManagerEvent, HandlerMa
461460
</StrictMode>
462461
);
463462

464-
setTimeout(() => ReactDOM.render(dialog, ModalManager.getOrCreateContainer()), 0);
463+
ModalManager.getOrCreateRoot().render(dialog);
465464
} else {
466465
// This is safe to call repeatedly if we happen to do that
467-
ReactDOM.unmountComponentAtNode(ModalManager.getOrCreateContainer());
466+
ModalManager.getOrCreateRoot().render(<></>);
468467
}
469468
}
470469
}

‎test/unit-tests/components/structures/MatrixChat-test.tsx‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ import { DRAFT_LAST_CLEANUP_KEY } from "../../../../src/DraftCleaner";
6262
import { UIFeature } from "../../../../src/settings/UIFeature";
6363
import AutoDiscoveryUtils from "../../../../src/utils/AutoDiscoveryUtils";
6464
import { ValidatedServerConfig } from "../../../../src/utils/ValidatedServerConfig";
65+
import Modal from "../../../../src/Modal.tsx";
6566

6667
jest.mock("matrix-js-sdk/src/oidc/authorize", () => ({
6768
completeAuthorizationCodeGrant: jest.fn(),
@@ -1514,7 +1515,9 @@ describe("<MatrixChat />", () => {
15141515

15151516
describe("when key backup failed", () => {
15161517
it("should show the new recovery method dialog", async () => {
1518+
const spy = jest.spyOn(Modal, "createDialogAsync");
15171519
jest.mock("../../../../src/async-components/views/dialogs/security/NewRecoveryMethodDialog", () => ({
1520+
__test: true,
15181521
__esModule: true,
15191522
default: () => <span>mocked dialog</span>,
15201523
}));
@@ -1526,7 +1529,8 @@ describe("<MatrixChat />", () => {
15261529
});
15271530
await flushPromises();
15281531
mockClient.emit(CryptoEvent.KeyBackupFailed, "error code");
1529-
await waitFor(() => expect(screen.getByText("mocked dialog")).toBeInTheDocument());
1532+
await waitFor(() => expect(spy).toHaveBeenCalledTimes(1));
1533+
expect(await spy.mock.lastCall![0]).toEqual(expect.objectContaining({ __test: true }));
15301534
});
15311535
});
15321536
});

‎test/unit-tests/components/structures/auth/ForgotPassword-test.tsx‎

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ Please see LICENSE files in the repository root for full details.
88

99
import React from "react";
1010
import { mocked } from "jest-mock";
11-
import { act, render, RenderResult, screen } from "jest-matrix-react";
11+
import { act, render, RenderResult, screen, waitFor } from "jest-matrix-react";
1212
import userEvent from "@testing-library/user-event";
1313
import { MatrixClient, createClient } from "matrix-js-sdk/src/matrix";
1414

@@ -47,14 +47,12 @@ describe("<ForgotPassword>", () => {
4747
};
4848

4949
const click = async (element: Element): Promise<void> => {
50-
await act(async () => {
51-
await userEvent.click(element, { delay: null });
52-
});
50+
await userEvent.click(element, { delay: null });
5351
};
5452

5553
const itShouldCloseTheDialogAndShowThePasswordInput = (): void => {
56-
it("should close the dialog and show the password input", () => {
57-
expect(screen.queryByText("Verify your email to continue")).not.toBeInTheDocument();
54+
it("should close the dialog and show the password input", async () => {
55+
await waitFor(() => expect(screen.queryByText("Verify your email to continue")).not.toBeInTheDocument());
5856
expect(screen.getByText("Reset your password")).toBeInTheDocument();
5957
});
6058
};
@@ -314,7 +312,7 @@ describe("<ForgotPassword>", () => {
314312
});
315313
});
316314

317-
it("should send the new password and show the click validation link dialog", () => {
315+
it("should send the new password and show the click validation link dialog", async () => {
318316
expect(client.setPassword).toHaveBeenCalledWith(
319317
{
320318
type: "m.login.email.identity",
@@ -326,15 +324,15 @@ describe("<ForgotPassword>", () => {
326324
testPassword,
327325
false,
328326
);
329-
expect(screen.getByText("Verify your email to continue")).toBeInTheDocument();
327+
await expect(
328+
screen.findByText("Verify your email to continue"),
329+
).resolves.toBeInTheDocument();
330330
expect(screen.getByText(testEmail)).toBeInTheDocument();
331331
});
332332

333333
describe("and dismissing the dialog by clicking the background", () => {
334334
beforeEach(async () => {
335-
await act(async () => {
336-
await userEvent.click(screen.getByTestId("dialog-background"), { delay: null });
337-
});
335+
await userEvent.click(await screen.findByTestId("dialog-background"), { delay: null });
338336
await waitEnoughCyclesForModal({
339337
useFakeTimers: true,
340338
});
@@ -345,7 +343,7 @@ describe("<ForgotPassword>", () => {
345343

346344
describe("and dismissing the dialog", () => {
347345
beforeEach(async () => {
348-
await click(screen.getByLabelText("Close dialog"));
346+
await click(await screen.findByLabelText("Close dialog"));
349347
await waitEnoughCyclesForModal({
350348
useFakeTimers: true,
351349
});
@@ -356,14 +354,16 @@ describe("<ForgotPassword>", () => {
356354

357355
describe("and clicking »Re-enter email address«", () => {
358356
beforeEach(async () => {
359-
await click(screen.getByText("Re-enter email address"));
357+
await click(await screen.findByText("Re-enter email address"));
360358
await waitEnoughCyclesForModal({
361359
useFakeTimers: true,
362360
});
363361
});
364362

365-
it("should close the dialog and go back to the email input", () => {
366-
expect(screen.queryByText("Verify your email to continue")).not.toBeInTheDocument();
363+
it("should close the dialog and go back to the email input", async () => {
364+
await waitFor(() =>
365+
expect(screen.queryByText("Verify your email to continue")).not.toBeInTheDocument(),
366+
);
367367
expect(screen.queryByText("Enter your email to reset password")).toBeInTheDocument();
368368
});
369369
});
@@ -397,11 +397,11 @@ describe("<ForgotPassword>", () => {
397397
});
398398

399399
it("should show the sign out warning dialog", async () => {
400-
expect(
401-
screen.getByText(
400+
await expect(
401+
screen.findByText(
402402
"Signing out your devices will delete the message encryption keys stored on them, making encrypted chat history unreadable.",
403403
),
404-
).toBeInTheDocument();
404+
).resolves.toBeInTheDocument();
405405

406406
// confirm dialog
407407
await click(screen.getByText("Continue"));

‎test/unit-tests/components/views/dialogs/ConfirmRedactDialog-test.tsx‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ Please see LICENSE files in the repository root for full details.
88

99
import { Feature, ServerSupport } from "matrix-js-sdk/src/feature";
1010
import { MatrixClient, MatrixEvent, RelationType } from "matrix-js-sdk/src/matrix";
11-
import { screen } from "jest-matrix-react";
11+
import { screen, act } from "jest-matrix-react";
1212
import userEvent from "@testing-library/user-event";
1313

1414
import { flushPromises, mkEvent, stubClient } from "../../../../test-utils";
@@ -31,12 +31,12 @@ describe("ConfirmRedactDialog", () => {
3131
};
3232

3333
const confirmDeleteVoiceBroadcastStartedEvent = async () => {
34-
createRedactEventDialog({ mxEvent });
34+
act(() => createRedactEventDialog({ mxEvent }));
3535
// double-flush promises required for the dialog to show up
3636
await flushPromises();
3737
await flushPromises();
3838

39-
await userEvent.click(screen.getByTestId("dialog-primary-button"));
39+
await userEvent.click(await screen.findByTestId("dialog-primary-button"));
4040
};
4141

4242
beforeEach(() => {

‎test/unit-tests/components/views/messages/DateSeparator-test.tsx‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ Please see LICENSE files in the repository root for full details.
88

99
import React from "react";
1010
import { mocked } from "jest-mock";
11-
import { fireEvent, render, screen } from "jest-matrix-react";
11+
import { fireEvent, render, screen, waitFor } from "jest-matrix-react";
1212
import { TimestampToEventResponse, ConnectionError, HTTPError, MatrixError } from "matrix-js-sdk/src/matrix";
1313

1414
import dispatcher from "../../../../../src/dispatcher/dispatcher";
@@ -291,7 +291,9 @@ describe("DateSeparator", () => {
291291
// The submit debug logs option should *NOT* be shown for network errors.
292292
//
293293
// We have to use `queryBy` so that it can return `null` for something that does not exist.
294-
expect(screen.queryByTestId("jump-to-date-error-submit-debug-logs-button")).not.toBeInTheDocument();
294+
await waitFor(() =>
295+
expect(screen.queryByTestId("jump-to-date-error-submit-debug-logs-button")).not.toBeInTheDocument(),
296+
);
295297
});
296298
});
297299
});

‎test/unit-tests/components/views/rooms/wysiwyg_composer/utils/message-test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -417,7 +417,7 @@ describe("message", () => {
417417
expect(mockClient.sendMessage).toHaveBeenCalledTimes(0);
418418
expect(mockClient.cancelPendingEvent).toHaveBeenCalledTimes(1);
419419
expect(mockCreateRedactEventDialog).toHaveBeenCalledTimes(1);
420-
expect(spyDispatcher).toHaveBeenCalledTimes(0);
420+
expect(spyDispatcher).toHaveBeenCalledTimes(1);
421421
});
422422

423423
it("Should do nothing if the content is unmodified", async () => {

‎test/unit-tests/components/views/settings/JoinRuleSettings-test.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,7 @@ describe("<JoinRuleSettings />", () => {
202202

203203
await flushPromises();
204204

205-
expect(within(dialog).getByText("Loading new room")).toBeInTheDocument();
205+
await expect(within(dialog).findByText("Loading new room")).resolves.toBeInTheDocument();
206206

207207
// "create" our new room, have it come thru sync
208208
client.getRoom.mockImplementation((id) => {
@@ -250,7 +250,7 @@ describe("<JoinRuleSettings />", () => {
250250

251251
await flushPromises();
252252

253-
expect(within(dialog).getByText("Loading new room")).toBeInTheDocument();
253+
await expect(within(dialog).findByText("Loading new room")).resolves.toBeInTheDocument();
254254

255255
// "create" our new room, have it come thru sync
256256
client.getRoom.mockImplementation((id) => {

‎test/unit-tests/components/views/settings/tabs/user/SessionManagerTab-test.tsx‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,7 @@ describe("<SessionManagerTab />", () => {
119119
const mockVerificationRequest = {
120120
cancel: jest.fn(),
121121
on: jest.fn(),
122+
off: jest.fn(),
122123
} as unknown as VerificationRequest;
123124

124125
const mockCrypto = mocked({

‎test/unit-tests/utils/media/requestMediaPermissions-test.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,9 @@ describe("requestMediaPermissions", () => {
1919
const audioStream = {} as MediaStream;
2020

2121
const itShouldLogTheErrorAndShowTheNoMediaPermissionsModal = () => {
22-
it("should log the error and show the »No media permissions« modal", () => {
22+
it("should log the error and show the »No media permissions« modal", async () => {
2323
expect(logger.log).toHaveBeenCalledWith("Failed to list userMedia devices", error);
24-
screen.getByText("No media permissions");
24+
await screen.findByText("No media permissions");
2525
});
2626
};
2727

0 commit comments

Comments
 (0)