From 257082e699e121f532d395047e0bb781ec83fd20 Mon Sep 17 00:00:00 2001 From: Sebastian Malton Date: Wed, 16 Nov 2022 15:48:35 -0500 Subject: [PATCH] Remove Singleton from BaseStore to remove global shared state Signed-off-by: Sebastian Malton --- src/common/__tests__/base-store.test.ts | 5 +-- src/common/base-store.ts | 5 ++- .../cluster-store/cluster-store.injectable.ts | 14 +++------ src/common/hotbars/store.injectable.ts | 12 +++---- .../user-store/user-store.injectable.ts | 12 +++---- src/common/utils/collection-functions.ts | 5 ++- src/common/utils/random-bytes.injectable.ts | 16 ++++++++++ src/common/utils/singleton.ts | 7 +++-- src/common/weblink-store.injectable.ts | 7 +---- ...ile-system-provisioner-store.injectable.ts | 18 +++++------ .../file-system-provisioner-store.ts | 25 ++++++++------- src/extensions/extension-store.ts | 31 +++++++++++++++++++ .../extensions-store.injectable.ts | 9 +----- src/renderer/bootstrap.tsx | 3 -- 14 files changed, 97 insertions(+), 72 deletions(-) create mode 100644 src/common/utils/random-bytes.injectable.ts diff --git a/src/common/__tests__/base-store.test.ts b/src/common/__tests__/base-store.test.ts index e6b1c2d1d2..9a4d512062 100644 --- a/src/common/__tests__/base-store.test.ts +++ b/src/common/__tests__/base-store.test.ts @@ -82,8 +82,6 @@ describe("BaseStore", () => { mainDi.override(directoryForUserDataInjectable, () => "some-user-data-directory"); mainDi.permitSideEffects(getConfigurationFileModelInjectable); - TestStore.resetInstance(); - const mockOpts = { "some-user-data-directory": { "test-store.json": JSON.stringify({}), @@ -92,13 +90,12 @@ describe("BaseStore", () => { mockFs(mockOpts); - store = TestStore.createInstance(); + store = new TestStore(); }); afterEach(() => { mockFs.restore(); store.disableSync(); - TestStore.resetInstance(); }); describe("persistence", () => { diff --git a/src/common/base-store.ts b/src/common/base-store.ts index 92383b328d..5372d34482 100644 --- a/src/common/base-store.ts +++ b/src/common/base-store.ts @@ -10,7 +10,7 @@ import { ipcMain, ipcRenderer } from "electron"; import type { IEqualsComparer } from "mobx"; import { makeObservable, reaction, runInAction } from "mobx"; import type { Disposer } from "./utils"; -import { Singleton, toJS } from "./utils"; +import { toJS } from "./utils"; import logger from "../main/logger"; import { broadcastMessage, ipcMainOn, ipcRendererOn } from "./ipc"; import isEqual from "lodash/isEqual"; @@ -31,14 +31,13 @@ export interface BaseStoreParams extends ConfOptions { /** * Note: T should only contain base JSON serializable types. */ -export abstract class BaseStore extends Singleton { +export abstract class BaseStore { protected storeConfig?: Config; protected syncDisposers: Disposer[] = []; readonly displayName: string = this.constructor.name; protected constructor(protected params: BaseStoreParams) { - super(); makeObservable(this); if (ipcRenderer) { diff --git a/src/common/cluster-store/cluster-store.injectable.ts b/src/common/cluster-store/cluster-store.injectable.ts index 3e7cf86c53..3904eb3d73 100644 --- a/src/common/cluster-store/cluster-store.injectable.ts +++ b/src/common/cluster-store/cluster-store.injectable.ts @@ -11,15 +11,11 @@ import emitAppEventInjectable from "../app-event-bus/emit-event.injectable"; const clusterStoreInjectable = getInjectable({ id: "cluster-store", - instantiate: (di) => { - ClusterStore.resetInstance(); - - return ClusterStore.createInstance({ - createCluster: di.inject(createClusterInjectionToken), - readClusterConfigSync: di.inject(readClusterConfigSyncInjectable), - emitAppEvent: di.inject(emitAppEventInjectable), - }); - }, + instantiate: (di) => new ClusterStore({ + createCluster: di.inject(createClusterInjectionToken), + readClusterConfigSync: di.inject(readClusterConfigSyncInjectable), + emitAppEvent: di.inject(emitAppEventInjectable), + }), causesSideEffects: true, }); diff --git a/src/common/hotbars/store.injectable.ts b/src/common/hotbars/store.injectable.ts index ace13b8be4..23ef120ed9 100644 --- a/src/common/hotbars/store.injectable.ts +++ b/src/common/hotbars/store.injectable.ts @@ -10,14 +10,10 @@ import loggerInjectable from "../logger.injectable"; const hotbarStoreInjectable = getInjectable({ id: "hotbar-store", - instantiate: (di) => { - HotbarStore.resetInstance(); - - return HotbarStore.createInstance({ - catalogCatalogEntity: di.inject(catalogCatalogEntityInjectable), - logger: di.inject(loggerInjectable), - }); - }, + instantiate: (di) => new HotbarStore({ + catalogCatalogEntity: di.inject(catalogCatalogEntityInjectable), + logger: di.inject(loggerInjectable), + }), causesSideEffects: true, }); diff --git a/src/common/user-store/user-store.injectable.ts b/src/common/user-store/user-store.injectable.ts index 4e01cc50eb..d593ea052d 100644 --- a/src/common/user-store/user-store.injectable.ts +++ b/src/common/user-store/user-store.injectable.ts @@ -10,14 +10,10 @@ import emitAppEventInjectable from "../app-event-bus/emit-event.injectable"; const userStoreInjectable = getInjectable({ id: "user-store", - instantiate: (di) => { - UserStore.resetInstance(); - - return UserStore.createInstance({ - selectedUpdateChannel: di.inject(selectedUpdateChannelInjectable), - emitAppEvent: di.inject(emitAppEventInjectable), - }); - }, + instantiate: (di) => new UserStore({ + selectedUpdateChannel: di.inject(selectedUpdateChannelInjectable), + emitAppEvent: di.inject(emitAppEventInjectable), + }), causesSideEffects: true, }); diff --git a/src/common/utils/collection-functions.ts b/src/common/utils/collection-functions.ts index 0702230464..d20383c29a 100644 --- a/src/common/utils/collection-functions.ts +++ b/src/common/utils/collection-functions.ts @@ -52,7 +52,10 @@ export function getOrInsertSet(map: Map>, key: K): Set { * Like `getOrInsert` but with delayed creation of the item. Which is useful * if it is very expensive to create the initial value. */ -export function getOrInsertWith(map: Map, key: K, builder: () => V): V { +export function getOrInsertWith(map: Map, key: K, builder: () => V): V; +export function getOrInsertWith(map: Map | WeakMap, key: K, builder: () => V): V; + +export function getOrInsertWith(map: Map | WeakMap, key: K, builder: () => V): V { if (!map.has(key)) { map.set(key, builder()); } diff --git a/src/common/utils/random-bytes.injectable.ts b/src/common/utils/random-bytes.injectable.ts new file mode 100644 index 0000000000..3f00ca525d --- /dev/null +++ b/src/common/utils/random-bytes.injectable.ts @@ -0,0 +1,16 @@ +/** + * Copyright (c) OpenLens Authors. All rights reserved. + * Licensed under MIT License. See LICENSE in root directory for more information. + */ +import { getInjectable } from "@ogre-tools/injectable"; +import { randomBytes } from "crypto"; +import { promisify } from "util"; + +export type RandomBytes = (size: number) => Promise; + +const randomBytesInjectable = getInjectable({ + id: "random-bytes", + instantiate: (): RandomBytes => promisify(randomBytes), +}); + +export default randomBytesInjectable; diff --git a/src/common/utils/singleton.ts b/src/common/utils/singleton.ts index d60fdb0490..0dea1f7526 100644 --- a/src/common/utils/singleton.ts +++ b/src/common/utils/singleton.ts @@ -3,10 +3,13 @@ * Licensed under MIT License. See LICENSE in root directory for more information. */ -interface StaticThis { new(...args: R): T } +export interface StaticThis { new(...args: R): T } +/** + * @deprecated This is a form of global shared state + */ export class Singleton { - private static instances = new WeakMap(); + private static readonly instances = new WeakMap(); private static creating = ""; constructor() { diff --git a/src/common/weblink-store.injectable.ts b/src/common/weblink-store.injectable.ts index 4aca7dce2a..1e6497f2b1 100644 --- a/src/common/weblink-store.injectable.ts +++ b/src/common/weblink-store.injectable.ts @@ -7,12 +7,7 @@ import { WeblinkStore } from "./weblink-store"; const weblinkStoreInjectable = getInjectable({ id: "weblink-store", - - instantiate: () => { - WeblinkStore.resetInstance(); - - return WeblinkStore.createInstance(); - }, + instantiate: () => new WeblinkStore(), }); export default weblinkStoreInjectable; diff --git a/src/extensions/extension-loader/file-system-provisioner-store/file-system-provisioner-store.injectable.ts b/src/extensions/extension-loader/file-system-provisioner-store/file-system-provisioner-store.injectable.ts index a0951baa8f..ad601307b9 100644 --- a/src/extensions/extension-loader/file-system-provisioner-store/file-system-provisioner-store.injectable.ts +++ b/src/extensions/extension-loader/file-system-provisioner-store/file-system-provisioner-store.injectable.ts @@ -5,19 +5,19 @@ import { getInjectable } from "@ogre-tools/injectable"; import { FileSystemProvisionerStore } from "./file-system-provisioner-store"; import directoryForExtensionDataInjectable from "./directory-for-extension-data.injectable"; +import ensureDirectoryInjectable from "../../../common/fs/ensure-dir.injectable"; +import joinPathsInjectable from "../../../common/path/join-paths.injectable"; +import randomBytesInjectable from "../../../common/utils/random-bytes.injectable"; const fileSystemProvisionerStoreInjectable = getInjectable({ id: "file-system-provisioner-store", - instantiate: (di) => { - FileSystemProvisionerStore.resetInstance(); - - return FileSystemProvisionerStore.createInstance({ - directoryForExtensionData: di.inject(directoryForExtensionDataInjectable), - }); - }, - - causesSideEffects: true, + instantiate: (di) => new FileSystemProvisionerStore({ + directoryForExtensionData: di.inject(directoryForExtensionDataInjectable), + ensureDirectory: di.inject(ensureDirectoryInjectable), + joinPaths: di.inject(joinPathsInjectable), + randomBytes: di.inject(randomBytesInjectable), + }), }); export default fileSystemProvisionerStoreInjectable; diff --git a/src/extensions/extension-loader/file-system-provisioner-store/file-system-provisioner-store.ts b/src/extensions/extension-loader/file-system-provisioner-store/file-system-provisioner-store.ts index fab21df3cc..7753d1a9ab 100644 --- a/src/extensions/extension-loader/file-system-provisioner-store/file-system-provisioner-store.ts +++ b/src/extensions/extension-loader/file-system-provisioner-store/file-system-provisioner-store.ts @@ -3,28 +3,31 @@ * Licensed under MIT License. See LICENSE in root directory for more information. */ -import { randomBytes } from "crypto"; import { SHA256 } from "crypto-js"; -import fse from "fs-extra"; import { action, makeObservable, observable } from "mobx"; -import path from "path"; import { BaseStore } from "../../../common/base-store"; import type { LensExtensionId } from "../../lens-extension"; -import { getOrInsertWith, toJS } from "../../../common/utils"; +import { getOrInsertWithAsync, toJS } from "../../../common/utils"; +import type { EnsureDirectory } from "../../../common/fs/ensure-dir.injectable"; +import type { JoinPaths } from "../../../common/path/join-paths.injectable"; +import type { RandomBytes } from "../../../common/utils/random-bytes.injectable"; interface FSProvisionModel { extensions: Record; // extension names to paths } interface Dependencies { - directoryForExtensionData: string; + readonly directoryForExtensionData: string; + ensureDirectory: EnsureDirectory; + joinPaths: JoinPaths; + randomBytes: RandomBytes; } export class FileSystemProvisionerStore extends BaseStore { readonly displayName = "FilesystemProvisionerStore"; - registeredExtensions = observable.map(); + readonly registeredExtensions = observable.map(); - constructor(private dependencies: Dependencies) { + constructor(private readonly dependencies: Dependencies) { super({ configName: "lens-filesystem-provisioner-store", accessPropertiesByDotNotation: false, // To make dots safe in cluster context names @@ -41,14 +44,14 @@ export class FileSystemProvisionerStore extends BaseStore { * @returns path to the folder that the extension can safely write files to. */ async requestDirectory(extensionName: string): Promise { - const dirPath = getOrInsertWith(this.registeredExtensions, extensionName, () => { - const salt = randomBytes(32).toString("hex"); + const dirPath = await getOrInsertWithAsync(this.registeredExtensions, extensionName, async () => { + const salt = (await this.dependencies.randomBytes(32)).toString("hex"); const hashedName = SHA256(`${extensionName}/${salt}`).toString(); - return path.resolve(this.dependencies.directoryForExtensionData, hashedName); + return this.dependencies.joinPaths(this.dependencies.directoryForExtensionData, hashedName); }); - await fse.ensureDir(dirPath); + await this.dependencies.ensureDirectory(dirPath); return dirPath; } diff --git a/src/extensions/extension-store.ts b/src/extensions/extension-store.ts index 4217576d2f..868247260e 100644 --- a/src/extensions/extension-store.ts +++ b/src/extensions/extension-store.ts @@ -7,8 +7,39 @@ import { BaseStore } from "../common/base-store"; import * as path from "path"; import type { LensExtension } from "./lens-extension"; import assert from "assert"; +import type { StaticThis } from "../common/utils"; +import { getOrInsertWith } from "../common/utils"; export abstract class ExtensionStore extends BaseStore { + private static readonly instances = new WeakMap>(); + + /** + * @deprecated This is a form of global shared state. Just call `new Store(...)` + */ + static createInstance, R extends any[]>(this: StaticThis, ...args: R): T { + return getOrInsertWith(ExtensionStore.instances, this, () => new this(...args)) as T; + } + + /** + * @deprecated This is a form of global shared state. Just call `new Store(...)` + */ + static getInstance(this: StaticThis, strict?: true): T; + static getInstance(this: StaticThis, strict: false): T | undefined; + static getInstance(this: StaticThis, strict = true): T | undefined { + if (!ExtensionStore.instances.has(this) && strict) { + throw new TypeError(`instance of ${this.name} is not created`); + } + + return ExtensionStore.instances.get(this) as (T | undefined); + } + + /** + * @deprecated This is a form of global shared state. Just call `new Store(...)` + */ + static resetInstance() { + ExtensionStore.instances.delete(this); + } + readonly displayName = "ExtensionStore"; protected extension?: LensExtension; diff --git a/src/extensions/extensions-store/extensions-store.injectable.ts b/src/extensions/extensions-store/extensions-store.injectable.ts index edf84e4b66..927444e9e2 100644 --- a/src/extensions/extensions-store/extensions-store.injectable.ts +++ b/src/extensions/extensions-store/extensions-store.injectable.ts @@ -7,14 +7,7 @@ import { ExtensionsStore } from "./extensions-store"; const extensionsStoreInjectable = getInjectable({ id: "extensions-store", - - instantiate: () => { - ExtensionsStore.resetInstance(); - - return ExtensionsStore.createInstance(); - }, - - causesSideEffects: true, + instantiate: () => new ExtensionsStore(), }); export default extensionsStoreInjectable; diff --git a/src/renderer/bootstrap.tsx b/src/renderer/bootstrap.tsx index ea3899b566..735d349ac7 100644 --- a/src/renderer/bootstrap.tsx +++ b/src/renderer/bootstrap.tsx @@ -19,7 +19,6 @@ import { DefaultProps } from "./mui-base-theme"; import configurePackages from "../common/configure-packages"; import * as initializers from "./initializers"; import logger from "../common/logger"; -import { WeblinkStore } from "../common/weblink-store"; import { registerCustomThemes } from "./components/monaco-editor"; import { getDi } from "./getDi"; import { DiContextProvider } from "@ogre-tools/injectable-react"; @@ -130,8 +129,6 @@ export async function bootstrap(di: DiContainer) { // TODO: Remove temporal dependencies di.inject(themeStoreInjectable); - WeblinkStore.createInstance(); - const extensionInstallationStateStore = di.inject(extensionInstallationStateStoreInjectable); extensionInstallationStateStore.bindIpcListeners();