From e5333672c80f6aaa99198717f7601e991a35182f Mon Sep 17 00:00:00 2001 From: colin-grant-work Date: Thu, 11 Aug 2022 11:32:25 -0600 Subject: [PATCH] Remove `widgetPromises` from `WidgetManager` (#11555) Fixes a bug in `tryGetWidget`. `widgetPromises` and `widgets` had same content and lifecycle. --- CHANGELOG.md | 1 + .../core/src/browser/widget-manager.spec.ts | 59 +++++++++++++------ packages/core/src/browser/widget-manager.ts | 14 ++--- 3 files changed, 48 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66cd3e891066a..4d0c1e6a2cf50 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/packages/core/src/browser/widget-manager.spec.ts b/packages/core/src/browser/widget-manager.spec.ts index 5e583c6276731..2d0e953cbb697 100644 --- a/packages/core/src/browser/widget-manager.spec.ts +++ b/packages/core/src/browser/widget-manager.spec.ts @@ -13,15 +13,20 @@ // // 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; @@ -29,32 +34,36 @@ class TestWidgetFactory implements WidgetFactory { async createWidget(name: string): Promise { 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 - (result).disposed = new Signal(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', diff --git a/packages/core/src/browser/widget-manager.ts b/packages/core/src/browser/widget-manager.ts index 822c170464c51..2dda99f4b4cfe 100644 --- a/packages/core/src/browser/widget-manager.ts +++ b/packages/core/src/browser/widget-manager.ts @@ -114,7 +114,6 @@ export class WidgetManager { protected _cachedFactories: Map; protected readonly widgets = new Map(); - protected readonly widgetPromises = new Map>(); protected readonly pendingWidgetPromises = new Map>(); @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(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(key: string): MaybePromise | 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; } @@ -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 {