fix global storage change events for deletes ( #91582)

src/vs/base/parts/storage/common/storage.ts

@@ -27,7 +27,8 @@ export interface IUpdateRequest {
 }
 
 export interface IStorageItemsChangeEvent {
-	items: Map<string, string>;
+	changed?: Map<string, string>;
+	deleted?: Set<string>;
 }
 
 export interface IStorageDatabase {
@@ -104,10 +105,11 @@ export class Storage extends Disposable implements IStorage {
 		// items that change external require us to update our
 		// caches with the values. we just accept the value and
 		// emit an event if there is a change.
-		e.items.forEach((value, key) => this.accept(key, value));
+		e.changed?.forEach((value, key) => this.accept(key, value));
+		e.deleted?.forEach(key => this.accept(key, undefined));
 	}
 
-	private accept(key: string, value: string): void {
+	private accept(key: string, value: string | undefined): void {
 		if (this.state === StorageState.Closed) {
 			return; // Return early if we are already closed
 		}
@@ -315,4 +317,4 @@ export class InMemoryStorageDatabase implements IStorageDatabase {
 	close(): Promise<void> {
 		return Promise.resolve();
 	}
-}
\ No newline at end of file
+}

src/vs/base/parts/storage/test/node/storage.test.ts

@@ -124,28 +124,27 @@ suite('Storage Library', () => {
 		changes.clear();
 
 		// Nothing happens if changing to same value
-		const change = new Map<string, string>();
-		change.set('foo', 'bar');
-		database.fireDidChangeItemsExternal({ items: change });
+		const changed = new Map<string, string>();
+		changed.set('foo', 'bar');
+		database.fireDidChangeItemsExternal({ changed });
 		equal(changes.size, 0);
 
 		// Change is accepted if valid
-		change.set('foo', 'bar1');
-		database.fireDidChangeItemsExternal({ items: change });
+		changed.set('foo', 'bar1');
+		database.fireDidChangeItemsExternal({ changed });
 		ok(changes.has('foo'));
 		equal(storage.get('foo'), 'bar1');
 		changes.clear();
 
 		// Delete is accepted
-		change.set('foo', undefined!);
-		database.fireDidChangeItemsExternal({ items: change });
+		const deleted = new Set<string>(['foo']);
+		database.fireDidChangeItemsExternal({ deleted });
 		ok(changes.has('foo'));
-		equal(storage.get('foo', null!), null);
+		equal(storage.get('foo', undefined), undefined);
 		changes.clear();
 
 		// Nothing happens if changing to same value
-		change.set('foo', undefined!);
-		database.fireDidChangeItemsExternal({ items: change });
+		database.fireDidChangeItemsExternal({ deleted });
 		equal(changes.size, 0);
 
 		await storage.close();

src/vs/platform/storage/browser/storageService.ts

@@ -240,9 +240,36 @@ export class FileStorageDatabase extends Disposable implements IStorageDatabase
 	private async onDidStorageChangeExternal(): Promise<void> {
 		const items = await this.doGetItemsFromFile();
 
+		// pervious cache, diff for changes
+		let changed = new Map<string, string>();
+		let deleted = new Set<string>();
+		if (this.cache) {
+			items.forEach((value, key) => {
+				const existingValue = this.cache?.get(key);
+				if (existingValue !== value) {
+					changed.set(key, value);
+				}
+			});
+
+			this.cache.forEach((_, key) => {
+				if (!items.has(key)) {
+					deleted.add(key);
+				}
+			});
+		}
+
+		// no previous cache, consider all as changed
+		else {
+			changed = items;
+		}
+
+		// Update cache
 		this.cache = items;
 
-		this._onDidChangeItemsExternal.fire({ items });
+		// Emit as event as needed
+		if (changed.size > 0 || deleted.size > 0) {
+			this._onDidChangeItemsExternal.fire({ changed, deleted });
+		}
 	}
 
 	async getItems(): Promise<Map<string, string>> {

src/vs/platform/storage/node/storageIpc.ts

@@ -24,15 +24,16 @@ interface ISerializableUpdateRequest {
 }
 
 interface ISerializableItemsChangeEvent {
-	items: Item[];
+	changed?: Item[];
+	deleted?: Key[];
 }
 
 export class GlobalStorageDatabaseChannel extends Disposable implements IServerChannel {
 
 	private static readonly STORAGE_CHANGE_DEBOUNCE_TIME = 100;
 
-	private readonly _onDidChangeItems: Emitter<ISerializableItemsChangeEvent> = this._register(new Emitter<ISerializableItemsChangeEvent>());
-	readonly onDidChangeItems: Event<ISerializableItemsChangeEvent> = this._onDidChangeItems.event;
+	private readonly _onDidChangeItems = this._register(new Emitter<ISerializableItemsChangeEvent>());
+	readonly onDidChangeItems = this._onDidChangeItems.event;
 
 	private whenReady: Promise<void>;
 
@@ -99,15 +100,18 @@ export class GlobalStorageDatabaseChannel extends Disposable implements IServerC
 	}
 
 	private serializeEvents(events: IStorageChangeEvent[]): ISerializableItemsChangeEvent {
-		const items = new Map<Key, Value>();
+		const changed = new Map<Key, Value>();
+		const deleted = new Set<Key>();
 		events.forEach(event => {
 			const existing = this.storageMainService.get(event.key);
 			if (typeof existing === 'string') {
-				items.set(event.key, existing);
+				changed.set(event.key, existing);
+			} else {
+				deleted.add(event.key);
 			}
 		});
 
-		return { items: mapToSerializable(items) };
+		return { changed: mapToSerializable(changed), deleted: values(deleted) };
 	}
 
 	listen(_: unknown, event: string): Event<any> {
@@ -170,8 +174,11 @@ export class GlobalStorageDatabaseChannelClient extends Disposable implements IS
 	}
 
 	private onDidChangeItemsOnMain(e: ISerializableItemsChangeEvent): void {
-		if (Array.isArray(e.items)) {
-			this._onDidChangeItemsExternal.fire({ items: serializableToMap(e.items) });
+		if (Array.isArray(e.changed) || Array.isArray(e.deleted)) {
+			this._onDidChangeItemsExternal.fire({
+				changed: e.changed ? serializableToMap(e.changed) : undefined,
+				deleted: e.deleted ? new Set<string>(e.deleted) : undefined
+			});
 		}
 	}
 

src/vs/platform/storage/node/storageService.ts

@@ -4,7 +4,7 @@
  *--------------------------------------------------------------------------------------------*/
 
 import { Disposable, IDisposable, dispose } from 'vs/base/common/lifecycle';
-import { Event, Emitter } from 'vs/base/common/event';
+import { Emitter } from 'vs/base/common/event';
 import { ILogService, LogLevel } from 'vs/platform/log/common/log';
 import { IWorkspaceStorageChangeEvent, IStorageService, StorageScope, IWillSaveStateEvent, WillSaveStateReason, logStorage } from 'vs/platform/storage/common/storage';
 import { SQLiteStorageDatabase, ISQLiteStorageDatabaseLoggingOptions } from 'vs/base/parts/storage/node/storage';
@@ -25,11 +25,11 @@ export class NativeStorageService extends Disposable implements IStorageService
 	private static readonly WORKSPACE_STORAGE_NAME = 'state.vscdb';
 	private static readonly WORKSPACE_META_NAME = 'workspace.json';
 
-	private readonly _onDidChangeStorage: Emitter<IWorkspaceStorageChangeEvent> = this._register(new Emitter<IWorkspaceStorageChangeEvent>());
-	readonly onDidChangeStorage: Event<IWorkspaceStorageChangeEvent> = this._onDidChangeStorage.event;
+	private readonly _onDidChangeStorage = this._register(new Emitter<IWorkspaceStorageChangeEvent>());
+	readonly onDidChangeStorage = this._onDidChangeStorage.event;
 
-	private readonly _onWillSaveState: Emitter<IWillSaveStateEvent> = this._register(new Emitter<IWillSaveStateEvent>());
-	readonly onWillSaveState: Event<IWillSaveStateEvent> = this._onWillSaveState.event;
+	private readonly _onWillSaveState = this._register(new Emitter<IWillSaveStateEvent>());
+	readonly onWillSaveState = this._onWillSaveState.event;
 
 	private globalStorage: IStorage;