Remove widgetPromises from WidgetManager (#11555)

Fixes a bug in `tryGetWidget`.
`widgetPromises` and `widgets` had same content and lifecycle.

CHANGELOG.md

@@ -12,6 +12,7 @@
 
 - [core] `updateThemePreference` and `updateThemeFromPreference` removed from `CommonFrontendContribution`. Corresponding functionality as been moved to the respective theme service. `load` removed from `IconThemeService` [#11473](https://github.com/eclipse-theia/theia/issues/11473)
 - [core] updated `react` and `react-dom` dependencies to version 18, which introduce new root API for rendering (replaces ReactDOM.render). Since React no longer supports render callbacks, the `onRender` field from `ReactDialog` and `ReactWidget` was removed. [#11455](https://github.com/eclipse-theia/theia/pull/11455) - Contributed on behalf of STMicroelectronics
+- [core] removed `WidgetManager.widgetPromises`; use `WidgetManager.widgets` instead. [#11555](https://github.com/eclipse-theia/theia/pull/11555)
 
 ## v1.28.0 - 7/28/2022
 

packages/core/src/browser/widget-manager.spec.ts

@@ -13,48 +13,57 @@
 //
 // SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
 // *****************************************************************************
+
+import { enableJSDOM } from './test/jsdom';
+
+let disableJsDom = enableJSDOM();
 import { Container, ContainerModule } from 'inversify';
 import { expect } from 'chai';
 import { WidgetManager, WidgetFactory } from './widget-manager';
 import { Widget } from '@phosphor/widgets';
-import { Signal } from '@phosphor/signaling';
 import { ILogger } from '../common/logger';
 import { MockLogger } from '../common/test/mock-logger';
 import { bindContributionProvider } from '../common';
 
+disableJsDom();
+
 class TestWidgetFactory implements WidgetFactory {
 
     invocations = 0;
     id = 'test';
 
     async createWidget(name: string): Promise<Widget> {
         this.invocations++;
-        // create a mock Widget, since a real widget has deps to dom api
-        const result = {} as Widget;
+        const result = new Widget;
         result.id = name;
-        // eslint-disable-next-line @typescript-eslint/no-explicit-any
-        (<any>result).disposed = new Signal<Widget, void>(result);
         return result;
     }
 }
 
-let widgetManager: WidgetManager;
+/* eslint-disable no-unused-expressions */
+describe('widget-manager', () => {
+    let widgetManager: WidgetManager;
+    before(() => {
+        disableJsDom = enableJSDOM();
+    });
 
-before(() => {
-    const testContainer = new Container();
+    beforeEach(() => {
+        const testContainer = new Container();
 
-    const module = new ContainerModule((bind, unbind, isBound, rebind) => {
-        bind(ILogger).to(MockLogger);
-        bindContributionProvider(bind, WidgetFactory);
-        bind(WidgetFactory).toConstantValue(new TestWidgetFactory());
-        bind(WidgetManager).toSelf().inSingletonScope();
-    });
-    testContainer.load(module);
+        const module = new ContainerModule(bind => {
+            bind(ILogger).to(MockLogger);
+            bindContributionProvider(bind, WidgetFactory);
+            bind(WidgetFactory).toConstantValue(new TestWidgetFactory());
+            bind(WidgetManager).toSelf().inSingletonScope();
+        });
+        testContainer.load(module);
 
-    widgetManager = testContainer.get(WidgetManager);
-});
+        widgetManager = testContainer.get(WidgetManager);
+    });
 
-describe('widget-manager', () => {
+    after(() => {
+        disableJsDom();
+    });
 
     it('creates and caches widgets', async () => {
         const wA = await widgetManager.getOrCreateWidget('test', 'widgetA');
@@ -63,6 +72,20 @@ describe('widget-manager', () => {
         expect(wA).equals(await widgetManager.getOrCreateWidget('test', 'widgetA'));
     });
 
+    describe('tryGetWidget', () => {
+        it('Returns undefined if the widget has not been created', () => {
+            expect(widgetManager.tryGetWidget('test', 'widgetA')).to.be.undefined;
+        });
+        it('Returns undefined if the widget is created asynchronously and has not finished being created', () => {
+            widgetManager.getOrCreateWidget('test', 'widgetA');
+            expect(widgetManager.tryGetWidget('test', 'widgetA')).to.be.undefined;
+        });
+        it('Returns the widget if the widget is created asynchronously and has finished being created', async () => {
+            await widgetManager.getOrCreateWidget('test', 'widgetA');
+            expect(widgetManager.tryGetWidget('test', 'widgetA')).not.to.be.undefined;
+        });
+    });
+
     it('produces the same widget key regardless of object key order', () => {
         const options1 = {
             factoryId: 'a',

packages/core/src/browser/widget-manager.ts

@@ -114,7 +114,6 @@ export class WidgetManager {
 
     protected _cachedFactories: Map<string, WidgetFactory>;
     protected readonly widgets = new Map<string, Widget>();
-    protected readonly widgetPromises = new Map<string, MaybePromise<Widget>>();
     protected readonly pendingWidgetPromises = new Map<string, MaybePromise<Widget>>();
 
     @inject(ContributionProvider) @named(WidgetFactory)
@@ -156,10 +155,13 @@ export class WidgetManager {
      * @param options The widget factory specific information.
      *
      * @returns the widget if available, else `undefined`.
+     *
+     * The widget is 'available' if it has been created with the same {@link factoryId} and {@link options} by the {@link WidgetManager}.
+     * If the widget's creation is asynchronous, it is only available when the associated `Promise` is resolved.
      */
     tryGetWidget<T extends Widget>(factoryId: string, options?: any): T | undefined {
         const key = this.toKey({ factoryId, options });
-        const existing = this.widgetPromises.get(key);
+        const existing = this.widgets.get(key);
         if (existing instanceof Widget) {
             return existing as T;
         }
@@ -193,7 +195,7 @@ export class WidgetManager {
     }
 
     protected doGetWidget<T extends Widget>(key: string): MaybePromise<T> | undefined {
-        const pendingWidget = this.widgetPromises.get(key) ?? this.pendingWidgetPromises.get(key);
+        const pendingWidget = this.widgets.get(key) ?? this.pendingWidgetPromises.get(key);
         if (pendingWidget) {
             return pendingWidget as MaybePromise<T>;
         }
@@ -222,12 +224,8 @@ export class WidgetManager {
             this.pendingWidgetPromises.set(key, widgetPromise);
             const widget = await widgetPromise;
             await WaitUntilEvent.fire(this.onWillCreateWidgetEmitter, { factoryId, widget });
-            this.widgetPromises.set(key, widgetPromise);
             this.widgets.set(key, widget);
-            widget.disposed.connect(() => {
-                this.widgets.delete(key);
-                this.widgetPromises.delete(key);
-            });
+            widget.disposed.connect(() => this.widgets.delete(key));
             this.onDidCreateWidgetEmitter.fire({ factoryId, widget });
             return widget as T;
         } finally {