Skip to content

Commit 610e464

Browse files
author
Akos Kitta
committed
fix: do not let the React dialogs dispose on close
To workaround the React-based widget lifecycle issue: eclipse-theia/theia#12093. The dialog constructor is called once, hence the default `toDispose` listener will execute only once per app lifecycle. The default dialog close behavior (from Theia) will dispose the widget. On dialog reopen, the constructor won't be called again, as the dialog is injected as a singleton, but the React component has been unmounted on dialog dispose. Hence, they will be recreated. The `componentDidMount` method will be called, but the `componentWillUnmount` is not called anymore. The dialog is already disposed. It leads to a resource leak. Signed-off-by: Akos Kitta <[email protected]>
1 parent cf3a07f commit 610e464

File tree

1 file changed

+18
-35
lines changed
  • arduino-ide-extension/src/browser/theia/dialogs

1 file changed

+18
-35
lines changed

Diff for: arduino-ide-extension/src/browser/theia/dialogs/dialogs.tsx

+18-35
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,9 @@ import {
33
DialogProps,
44
} from '@theia/core/lib/browser/dialogs';
55
import { ReactDialog as TheiaReactDialog } from '@theia/core/lib/browser/dialogs/react-dialog';
6-
import { codiconArray, Message } from '@theia/core/lib/browser/widgets/widget';
7-
import {
8-
Disposable,
9-
DisposableCollection,
10-
} from '@theia/core/lib/common/disposable';
6+
import { codiconArray } from '@theia/core/lib/browser/widgets/widget';
7+
import type { Message } from '@theia/core/shared/@phosphor/messaging';
118
import { inject, injectable } from '@theia/core/shared/inversify';
12-
import * as React from '@theia/core/shared/react';
13-
import { createRoot } from '@theia/core/shared/react-dom/client';
149

1510
@injectable()
1611
export abstract class AbstractDialog<T> extends TheiaAbstractDialog<T> {
@@ -25,38 +20,26 @@ export abstract class AbstractDialog<T> extends TheiaAbstractDialog<T> {
2520

2621
@injectable()
2722
export abstract class ReactDialog<T> extends TheiaReactDialog<T> {
28-
protected override onUpdateRequest(msg: Message): void {
29-
// This is tricky to bypass the default Theia code.
30-
// Otherwise, there is a warning when opening the dialog for the second time.
31-
// You are calling ReactDOMClient.createRoot() on a container that has already been passed to createRoot() before. Instead, call root.render() on the existing root instead if you want to update it.
32-
const disposables = new DisposableCollection();
33-
if (!this.isMounted) {
34-
// toggle the `isMounted` logic for the time being of the super call so that the `createRoot` does not run
35-
this.isMounted = true;
36-
disposables.push(Disposable.create(() => (this.isMounted = false)));
37-
}
23+
private _isOnCloseRequestInProgress = false;
3824

39-
// Always unset the `contentNodeRoot` so there is no double update when calling super.
40-
const restoreContentNodeRoot = this.contentNodeRoot;
41-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
42-
(this.contentNodeRoot as any) = undefined;
43-
disposables.push(
44-
Disposable.create(() => (this.contentNodeRoot = restoreContentNodeRoot))
45-
);
25+
override dispose(): void {
26+
// There is a bug in Theia, and the React component's `componentWillUnmount` will not be called, as the Theia widget is already disposed when closing and reopening a dialog.
27+
// Widget lifecycle issue in Theia: https://github.com/eclipse-theia/theia/issues/12093
28+
// Bogus react widget lifecycle management PR: https://github.com/eclipse-theia/theia/pull/11687
29+
// Do not call super. Do not let the Phosphor widget to be disposed on dialog close.
30+
if (this._isOnCloseRequestInProgress) {
31+
// Do not let the widget dispose on close.
32+
return;
33+
}
34+
super.dispose();
35+
}
4636

37+
protected override onCloseRequest(message: Message): void {
38+
this._isOnCloseRequestInProgress = true;
4739
try {
48-
super.onUpdateRequest(msg);
40+
super.onCloseRequest(message);
4941
} finally {
50-
disposables.dispose();
51-
}
52-
53-
// Use the patched rendering.
54-
if (!this.isMounted) {
55-
this.contentNodeRoot = createRoot(this.contentNode);
56-
// Resetting the prop is missing from the Theia code.
57-
// https://github.com/eclipse-theia/theia/blob/v1.31.1/packages/core/src/browser/dialogs/react-dialog.tsx#L41-L47
58-
this.isMounted = true;
42+
this._isOnCloseRequestInProgress = false;
5943
}
60-
this.contentNodeRoot?.render(<>{this.render()}</>);
6144
}
6245
}

0 commit comments

Comments
 (0)