Add a disposable base class (#6445)

* Add a disposable base class
Fixes #6285

* PR feedback

Author: alexr00

package.json

@@ -3705,7 +3705,7 @@
 		"constants-browserify": "^1.0.0",
 		"crypto-browserify": "3.12.0",
 		"css-loader": "5.1.3",
-		"esbuild-loader": "2.10.0",
+		"esbuild-loader": "4.2.2",
 		"eslint": "7.22.0",
 		"eslint-cli": "1.1.1",
 		"eslint-plugin-import": "2.22.1",

src/api/api1.ts

@@ -5,6 +5,7 @@
 
 import * as vscode from 'vscode';
 import { APIState, PublishEvent } from '../@types/git';
+import { Disposable } from '../common/lifecycle';
 import Logger from '../common/logger';
 import { TernarySearchTree } from '../common/utils';
 import { API, IGit, PostCommitCommandsProvider, Repository, ReviewerCommentsProvider, TitleAndDescriptionProvider } from './api';
@@ -72,7 +73,7 @@ export const enum Status {
 	BOTH_MODIFIED,
 }
 
-export class GitApiImpl implements API, IGit, vscode.Disposable {
+export class GitApiImpl extends Disposable implements API, IGit {
 	private static _handlePool: number = 0;
 	private _providers = new Map<number, IGit>();
 
@@ -111,11 +112,6 @@ export class GitApiImpl implements API, IGit, vscode.Disposable {
 	private _onDidPublish = new vscode.EventEmitter<PublishEvent>();
 	readonly onDidPublish: vscode.Event<PublishEvent> = this._onDidPublish.event;
 
-	private _disposables: vscode.Disposable[];
-	constructor() {
-		this._disposables = [];
-	}
-
 	private _updateReposContext() {
 		const reposCount = Array.from(this._providers.values()).reduce((prev, current) => {
 			return prev + current.repositories.length;
@@ -128,17 +124,17 @@ export class GitApiImpl implements API, IGit, vscode.Disposable {
 		const handle = this._nextHandle();
 		this._providers.set(handle, provider);
 
-		this._disposables.push(provider.onDidCloseRepository(e => this._onDidCloseRepository.fire(e)));
-		this._disposables.push(provider.onDidOpenRepository(e => {
+		this._register(provider.onDidCloseRepository(e => this._onDidCloseRepository.fire(e)));
+		this._register(provider.onDidOpenRepository(e => {
 			Logger.appendLine(`Repository ${e.rootUri} has been opened`);
 			this._updateReposContext();
 			this._onDidOpenRepository.fire(e);
 		}));
 		if (provider.onDidChangeState) {
-			this._disposables.push(provider.onDidChangeState(e => this._onDidChangeState.fire(e)));
+			this._register(provider.onDidChangeState(e => this._onDidChangeState.fire(e)));
 		}
 		if (provider.onDidPublish) {
-			this._disposables.push(provider.onDidPublish(e => this._onDidPublish.fire(e)));
+			this._register(provider.onDidPublish(e => this._onDidPublish.fire(e)));
 		}
 
 		this._updateReposContext();
@@ -188,18 +184,13 @@ export class GitApiImpl implements API, IGit, vscode.Disposable {
 		return GitApiImpl._handlePool++;
 	}
 
-	dispose() {
-		this._disposables.forEach(disposable => disposable.dispose());
-	}
-
 	private _titleAndDescriptionProviders: Set<{ title: string, provider: TitleAndDescriptionProvider }> = new Set();
 	registerTitleAndDescriptionProvider(title: string, provider: TitleAndDescriptionProvider): vscode.Disposable {
 		const registeredValue = { title, provider };
 		this._titleAndDescriptionProviders.add(registeredValue);
-		const disposable = {
+		const disposable = this._register({
 			dispose: () => this._titleAndDescriptionProviders.delete(registeredValue)
-		};
-		this._disposables.push(disposable);
+		});
 		return disposable;
 	}
 
@@ -219,10 +210,9 @@ export class GitApiImpl implements API, IGit, vscode.Disposable {
 	registerReviewerCommentsProvider(title: string, provider: ReviewerCommentsProvider): vscode.Disposable {
 		const registeredValue = { title, provider };
 		this._reviewerCommentsProviders.add(registeredValue);
-		const disposable = {
+		const disposable = this._register({
 			dispose: () => this._reviewerCommentsProviders.delete(registeredValue)
-		};
-		this._disposables.push(disposable);
+		});
 		return disposable;
 	}
 

src/common/authentication.ts

@@ -15,9 +15,7 @@ export enum AuthProvider {
 }
 
 export class AuthenticationError extends Error {
-	name: string;
-	stack?: string;
-	constructor(public message: string) {
+	constructor(message: string) {
 		super(message);
 	}
 }

src/common/lifecycle.ts

@@ -0,0 +1,49 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the MIT License. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+import * as vscode from 'vscode';
+
+export function toDisposable(d: () => void): vscode.Disposable {
+	return { dispose: d };
+}
+
+export function combinedDisposable(disposables: vscode.Disposable[]): vscode.Disposable {
+	return toDisposable(() => disposeAll(disposables));
+}
+
+export function disposeAll(disposables: vscode.Disposable[]) {
+	while (disposables.length) {
+		const item = disposables.pop();
+		item?.dispose();
+	}
+}
+
+export abstract class Disposable {
+	protected _isDisposed = false;
+
+	private _disposables: vscode.Disposable[] = [];
+
+	public dispose(): any {
+		if (this._isDisposed) {
+			return;
+		}
+		this._isDisposed = true;
+		disposeAll(this._disposables);
+		this._disposables = [];
+	}
+
+	protected _register<T extends vscode.Disposable>(value: T): T {
+		if (this._isDisposed) {
+			value.dispose();
+		} else {
+			this._disposables.push(value);
+		}
+		return value;
+	}
+
+	protected get isDisposed() {
+		return this._isDisposed;
+	}
+}

src/common/logger.ts

@@ -3,16 +3,17 @@
  *  Licensed under the MIT License. See License.txt in the project root for license information.
  *--------------------------------------------------------------------------------------------*/
 import * as vscode from 'vscode';
+import { Disposable } from './lifecycle';
 
 export const PR_TREE = 'PullRequestTree';
 
-class Log {
-	private _outputChannel: vscode.LogOutputChannel;
-	private _disposable: vscode.Disposable;
-	private _activePerfMarkers: Map<string, number> = new Map();
+class Log extends Disposable {
+	private readonly _outputChannel: vscode.LogOutputChannel;
+	private readonly _activePerfMarkers: Map<string, number> = new Map();
 
 	constructor() {
-		this._outputChannel = vscode.window.createOutputChannel('GitHub Pull Request', { log: true });
+		super();
+		this._outputChannel = this._register(vscode.window.createOutputChannel('GitHub Pull Request', { log: true }));
 	}
 
 	public startPerfMarker(marker: string) {
@@ -59,12 +60,6 @@ class Log {
 	public error(message: any, component?: string) {
 		this._outputChannel.error(this.logString(message, component));
 	}
-
-	public dispose() {
-		if (this._disposable) {
-			this._disposable.dispose();
-		}
-	}
 }
 
 const Logger = new Log();

src/common/temporaryState.ts

@@ -3,8 +3,8 @@
  *  Licensed under the MIT License. See License.txt in the project root for license information.
  *--------------------------------------------------------------------------------------------*/
 import * as vscode from 'vscode';
+import { disposeAll } from './lifecycle';
 import Logger from './logger';
-import { dispose } from './utils';
 
 let tempState: TemporaryState | undefined;
 
@@ -13,17 +13,17 @@ export class TemporaryState extends vscode.Disposable {
 	private readonly disposables: vscode.Disposable[] = [];
 	private readonly persistInSessionDisposables: vscode.Disposable[] = [];
 
-	constructor(private _storageUri: vscode.Uri) {
-		super(() => this.disposables.forEach(disposable => disposable.dispose()));
+	constructor(private readonly _storageUri: vscode.Uri) {
+		super(() => disposeAll(this.disposables));
 	}
 
 	private get path(): vscode.Uri {
 		return vscode.Uri.joinPath(this._storageUri, this.SUBPATH);
 	}
 
-	dispose() {
-		dispose(this.disposables);
-		dispose(this.persistInSessionDisposables);
+	override dispose() {
+		disposeAll(this.disposables);
+		disposeAll(this.persistInSessionDisposables);
 	}
 
 	private addDisposable(disposable: vscode.Disposable, persistInSession: boolean) {

src/common/utils.ts

@@ -9,6 +9,7 @@ import dayjs from 'dayjs';
 import * as relativeTime from 'dayjs/plugin/relativeTime';
 import * as updateLocale from 'dayjs/plugin/updateLocale';
 import type { Disposable, Event, ExtensionContext, Uri } from 'vscode';
+import { combinedDisposable } from './lifecycle';
 // TODO: localization for webview needed
 
 dayjs.extend(relativeTime.default, {
@@ -65,19 +66,6 @@ export function uniqBy<T>(arr: T[], fn: (el: T) => string): T[] {
 	});
 }
 
-export function dispose<T extends Disposable>(disposables: T[]): T[] {
-	disposables.forEach(d => d.dispose());
-	return [];
-}
-
-export function toDisposable(d: () => void): Disposable {
-	return { dispose: d };
-}
-
-export function combinedDisposable(disposables: Disposable[]): Disposable {
-	return toDisposable(() => dispose(disposables));
-}
-
 export function anyEvent<T>(...events: Event<T>[]): Event<T> {
 	return (listener, thisArgs = null, disposables?) => {
 		const result = combinedDisposable(events.map(event => event(i => listener.call(thisArgs, i))));

src/common/webview.ts

@@ -5,6 +5,7 @@
 
 import * as vscode from 'vscode';
 import { commands } from './executeCommands';
+import { Disposable } from './lifecycle';
 
 export const PULL_REQUEST_OVERVIEW_VIEW_TYPE = 'PullRequestOverview';
 
@@ -29,16 +30,16 @@ export function getNonce() {
 	return text;
 }
 
-export class WebviewBase {
+export class WebviewBase extends Disposable {
 	protected _webview?: vscode.Webview;
-	protected _disposables: vscode.Disposable[] = [];
 
 	private _waitForReady: Promise<void>;
-	private _onIsReady: vscode.EventEmitter<void> = new vscode.EventEmitter();
+	private _onIsReady: vscode.EventEmitter<void> = this._register(new vscode.EventEmitter());
 
 	protected readonly MESSAGE_UNHANDLED: string = 'message not handled';
 
 	constructor() {
+		super();
 		this._waitForReady = new Promise(resolve => {
 			const disposable = this._onIsReady.event(() => {
 				disposable.dispose();
@@ -51,12 +52,9 @@ export class WebviewBase {
 		const disposable = this._webview?.onDidReceiveMessage(
 			async message => {
 				await this._onDidReceiveMessage(message as IRequestMessage<any>);
-			},
-			null,
-			this._disposables,
-		);
+			});
 		if (disposable) {
-			this._disposables.push(disposable);
+			this._register(disposable);
 		}
 	}
 
@@ -94,10 +92,6 @@ export class WebviewBase {
 		};
 		this._webview?.postMessage(reply);
 	}
-
-	public dispose() {
-		this._disposables.forEach(d => d.dispose());
-	}
 }
 
 export class WebviewViewBase extends WebviewBase {
@@ -122,7 +116,7 @@ export class WebviewViewBase extends WebviewBase {
 
 			localResourceRoots: [this._extensionUri],
 		};
-		this._disposables.push(this._view.onDidDispose(() => {
+		this._register(this._view.onDidDispose(() => {
 			this._webview = undefined;
 			this._view = undefined;
 		}));

src/experimentationService.ts

@@ -11,17 +11,23 @@ import {
 	IExperimentationTelemetry,
 	TargetPopulation,
 } from 'vscode-tas-client';
+import { Disposable } from './common/lifecycle';
 
 /* __GDPR__
 	"query-expfeature" : {
 		"ABExp.queriedFeature": { "classification": "SystemMetaData", "purpose": "FeatureInsight" }
 	}
 */
 
-export class ExperimentationTelemetry implements IExperimentationTelemetry {
+export class ExperimentationTelemetry extends Disposable implements IExperimentationTelemetry {
 	private sharedProperties: Record<string, string> = {};
 
-	constructor(private baseReporter: TelemetryReporter | undefined) { }
+	constructor(private baseReporter: TelemetryReporter | undefined) {
+		super();
+		if (baseReporter) {
+			this._register(baseReporter);
+		}
+	}
 
 	sendTelemetryEvent(eventName: string, properties?: Record<string, string>, measurements?: Record<string, number>) {
 		this.baseReporter?.sendTelemetryEvent(
@@ -56,10 +62,6 @@ export class ExperimentationTelemetry implements IExperimentationTelemetry {
 		}
 		this.sendTelemetryEvent(eventName, event);
 	}
-
-	async dispose(): Promise<any> {
-		return this.baseReporter?.dispose();
-	}
 }
 
 function getTargetPopulation(): TargetPopulation {

src/extension.ts

@@ -147,7 +147,7 @@ async function init(
 		}
 	});
 
-	const changesTree = new PullRequestChangesTreeDataProvider(context, git, reposManager);
+	const changesTree = new PullRequestChangesTreeDataProvider(git, reposManager);
 	context.subscriptions.push(changesTree);
 
 	const activePrViewCoordinator = new WebviewViewCoordinator(context);