Extract minimal dependencies over a god class to satisfy Interface Segregation Principle and make stubbing in unit testing easier

Co-authored-by: Mikko Aspiala <mikko.aspiala@gmail.com>
Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

src/extensions/extension-discovery/extension-discovery.injectable.ts

@@ -22,17 +22,21 @@ import { getInjectable } from "@ogre-tools/injectable";
 import { lifecycleEnum } from "@ogre-tools/injectable";
 import { ExtensionDiscovery } from "./extension-discovery";
 import extensionLoaderInjectable from "../extension-loader/extension-loader.injectable";
-import extensionInstallerInjectable from "../extension-installer/extension-installer.injectable";
 import isCompatibleExtensionInjectable from "./is-compatible-extension/is-compatible-extension.injectable";
 import isCompatibleBundledExtensionInjectable from "./is-compatible-bundled-extension/is-compatible-bundled-extension.injectable";
 import extensionsStoreInjectable from "../extensions-store/extensions-store.injectable";
 import extensionInstallationStateStoreInjectable from "../extension-installation-state-store/extension-installation-state-store.injectable";
+import installExtensionInjectable
+  from "../extension-installer/install-extension/install-extension.injectable";
+import extensionPackageRootDirectoryInjectable
+  from "../extension-installer/extension-package-root-directory/extension-package-root-directory.injectable";
+import installExtensionsInjectable
+  from "../extension-installer/install-extensions/install-extensions.injectable";
 
 const extensionDiscoveryInjectable = getInjectable({
   instantiate: (di) =>
     new ExtensionDiscovery({
       extensionLoader: di.inject(extensionLoaderInjectable),
-      extensionInstaller: di.inject(extensionInstallerInjectable),
       extensionsStore: di.inject(extensionsStoreInjectable),
 
       extensionInstallationStateStore: di.inject(
@@ -44,6 +48,10 @@ const extensionDiscoveryInjectable = getInjectable({
       ),
 
       isCompatibleExtension: di.inject(isCompatibleExtensionInjectable),
+
+      installExtension: di.inject(installExtensionInjectable),
+      installExtensions: di.inject(installExtensionsInjectable),
+      extensionPackageRootDirectory: di.inject(extensionPackageRootDirectoryInjectable),
     }),
 
   lifecycle: lifecycleEnum.singleton,

src/extensions/extension-discovery/extension-discovery.test.ts

@@ -28,7 +28,8 @@ import { Console } from "console";
 import { AppPaths } from "../../common/app-paths";
 import { getDiForUnitTesting } from "../getDiForUnitTesting";
 import extensionDiscoveryInjectable from "./extension-discovery.injectable";
-import extensionInstallerInjectable from "../extension-installer/extension-installer.injectable";
+import extensionPackageRootDirectoryInjectable from "../extension-installer/extension-package-root-directory/extension-package-root-directory.injectable";
+import installExtensionInjectable from "../extension-installer/install-extension/install-extension.injectable";
 
 jest.setTimeout(60_000);
 
@@ -63,16 +64,12 @@ describe("ExtensionDiscovery", () => {
   beforeEach(() => {
     const di = getDiForUnitTesting();
 
-    const extensionInstallerStub = {
-      installPackages: () => Promise.resolve(),
-      npm: () => Promise.resolve(),
-      extensionPackagesRoot: "some-extension-packages-root",
-      npmPath: "some-npm-path",
-      installPackage: jest.fn(),
-    };
+    di.override(installExtensionInjectable, () => () => Promise.resolve());
 
-    // @ts-ignore
-    di.override(extensionInstallerInjectable, () => extensionInstallerStub);
+    di.override(
+      extensionPackageRootDirectoryInjectable,
+      () => "some-extension-packages-root",
+    );
 
     extensionDiscovery = di.inject(extensionDiscoveryInjectable);
   });
@@ -80,7 +77,8 @@ describe("ExtensionDiscovery", () => {
   describe("with mockFs", () => {
     beforeEach(() => {
       mockFs({
-        [`${os.homedir()}/.k8slens/extensions/my-extension/package.json`]: JSON.stringify({
+        [`${os.homedir()}/.k8slens/extensions/my-extension/package.json`]:
+          JSON.stringify({
             name: "my-extension",
           }),
       });
@@ -110,10 +108,12 @@ describe("ExtensionDiscovery", () => {
 
       await extensionDiscovery.watchExtensions();
 
-      extensionDiscovery.events.on("add", extension => {
+      extensionDiscovery.events.on("add", (extension) => {
         expect(extension).toEqual({
           absolutePath: expect.any(String),
-          id: path.normalize("some-extension-packages-root/node_modules/my-extension/package.json"),
+          id: path.normalize(
+            "some-extension-packages-root/node_modules/my-extension/package.json",
+          ),
           isBundled: false,
           isEnabled: false,
           isCompatible: false,
@@ -128,11 +128,16 @@ describe("ExtensionDiscovery", () => {
         done();
       });
 
-      addHandler(path.join(extensionDiscovery.localFolderPath, "/my-extension/package.json"));
+      addHandler(
+        path.join(
+          extensionDiscovery.localFolderPath,
+          "/my-extension/package.json",
+        ),
+      );
     });
   });
 
-  it("doesn't emit add for added file under extension", async done => {
+  it("doesn't emit add for added file under extension", async (done) => {
     let addHandler: (filePath: string) => void;
 
     const mockWatchInstance: any = {
@@ -156,7 +161,12 @@ describe("ExtensionDiscovery", () => {
 
     extensionDiscovery.events.on("add", onAdd);
 
-    addHandler(path.join(extensionDiscovery.localFolderPath, "/my-extension/node_modules/dep/package.json"));
+    addHandler(
+      path.join(
+        extensionDiscovery.localFolderPath,
+        "/my-extension/node_modules/dep/package.json",
+      ),
+    );
 
     setTimeout(() => {
       expect(onAdd).not.toHaveBeenCalled();

src/extensions/extension-discovery/extension-discovery.ts

@@ -34,23 +34,25 @@ import {
 } from "../../common/ipc";
 import { toJS } from "../../common/utils";
 import logger from "../../main/logger";
-import type { ExtensionInstaller } from "../extension-installer/extension-installer";
 import type { ExtensionsStore } from "../extensions-store/extensions-store";
 import type { ExtensionLoader } from "../extension-loader";
 import type { LensExtensionId, LensExtensionManifest } from "../lens-extension";
 import { isProduction } from "../../common/vars";
 import type { ExtensionInstallationStateStore } from "../extension-installation-state-store/extension-installation-state-store";
+import type { PackageJson } from "type-fest";
 
 interface Dependencies {
   extensionLoader: ExtensionLoader;
-
-  extensionInstaller: ExtensionInstaller;
   extensionsStore: ExtensionsStore;
 
   extensionInstallationStateStore: ExtensionInstallationStateStore;
 
   isCompatibleBundledExtension: (manifest: LensExtensionManifest) => boolean;
   isCompatibleExtension: (manifest: LensExtensionManifest) => boolean;
+
+  installExtension: (name: string) => Promise<void>;
+  installExtensions: (packageJsonPath: string, packagesJson: PackageJson) => Promise<void>
+  extensionPackageRootDirectory: string;
 }
 
 export interface InstalledExtension {
@@ -124,11 +126,11 @@ export class ExtensionDiscovery {
   }
 
   get packageJsonPath(): string {
-    return path.join(this.dependencies.extensionInstaller.extensionPackagesRoot, manifestFilename);
+    return path.join(this.dependencies.extensionPackageRootDirectory, manifestFilename);
   }
 
   get inTreeTargetPath(): string {
-    return path.join(this.dependencies.extensionInstaller.extensionPackagesRoot, "extensions");
+    return path.join(this.dependencies.extensionPackageRootDirectory, "extensions");
   }
 
   get inTreeFolderPath(): string {
@@ -136,7 +138,7 @@ export class ExtensionDiscovery {
   }
 
   get nodeModulesPath(): string {
-    return path.join(this.dependencies.extensionInstaller.extensionPackagesRoot, "node_modules");
+    return path.join(this.dependencies.extensionPackageRootDirectory, "node_modules");
   }
 
   /**
@@ -222,7 +224,7 @@ export class ExtensionDiscovery {
           await fse.remove(extension.manifestPath);
 
           // Install dependencies for the new extension
-          await this.installPackage(extension.absolutePath);
+          await this.dependencies.installExtension(extension.absolutePath);
 
           this.extensions.set(extension.id, extension);
           logger.info(`${logModule} Added extension ${extension.manifest.name}`);
@@ -309,15 +311,12 @@ export class ExtensionDiscovery {
 
     this.loadStarted = true;
 
-    const extensionPackagesRoot =
-      this.dependencies.extensionInstaller.extensionPackagesRoot;
-
     logger.info(
-      `${logModule} loading extensions from ${extensionPackagesRoot}`,
+      `${logModule} loading extensions from ${this.dependencies.extensionPackageRootDirectory}`,
     );
 
     // fs.remove won't throw if path is missing
-    await fse.remove(path.join(extensionPackagesRoot, "package-lock.json"));
+    await fse.remove(path.join(this.dependencies.extensionPackageRootDirectory, "package-lock.json"));
 
     try {
       // Verify write access to static/extensions, which is needed for symlinking
@@ -413,7 +412,7 @@ export class ExtensionDiscovery {
     for (const extension of userExtensions) {
       if ((await fse.pathExists(extension.manifestPath)) === false) {
         try {
-          await this.installPackage(extension.absolutePath);
+          await this.dependencies.installExtension(extension.absolutePath);
         } catch (error) {
           const message = error.message || error || "unknown error";
           const { name, version } = extension.manifest;
@@ -436,11 +435,7 @@ export class ExtensionDiscovery {
       extensions.map(extension => [extension.manifest.name, extension.absolutePath]),
     );
 
-    return this.dependencies.extensionInstaller.installPackages(packageJsonPath, { dependencies });
-  }
-
-  async installPackage(name: string): Promise<void> {
-    return this.dependencies.extensionInstaller.installPackage(name);
+    return this.dependencies.installExtensions(packageJsonPath, { dependencies });
   }
 
   async loadBundledExtensions(): Promise<InstalledExtension[]> {

src/extensions/extension-installer/extension-package-root-directory/extension-package-root-directory.injectable.ts

@@ -0,0 +1,31 @@
+/**
+ * Copyright (c) 2021 OpenLens Authors
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy of
+ * this software and associated documentation files (the "Software"), to deal in
+ * the Software without restriction, including without limitation the rights to
+ * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of
+ * the Software, and to permit persons to whom the Software is furnished to do so,
+ * subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
+ * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
+ * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER
+ * IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+import { getInjectable, lifecycleEnum } from "@ogre-tools/injectable";
+import extensionInstallerInjectable from "../extension-installer.injectable";
+
+const extensionPackageRootDirectoryInjectable = getInjectable({
+  instantiate: (di) =>
+    di.inject(extensionInstallerInjectable).extensionPackagesRoot,
+  
+  lifecycle: lifecycleEnum.singleton,
+});
+
+export default extensionPackageRootDirectoryInjectable;

src/extensions/extension-installer/install-extension/install-extension.injectable.ts

@@ -0,0 +1,30 @@
+/**
+ * Copyright (c) 2021 OpenLens Authors
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy of
+ * this software and associated documentation files (the "Software"), to deal in
+ * the Software without restriction, including without limitation the rights to
+ * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of
+ * the Software, and to permit persons to whom the Software is furnished to do so,
+ * subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
+ * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
+ * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER
+ * IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+import { getInjectable, lifecycleEnum } from "@ogre-tools/injectable";
+import extensionInstallerInjectable from "../extension-installer.injectable";
+
+const installExtensionInjectable = getInjectable({
+  instantiate: (di) => di.inject(extensionInstallerInjectable).installPackage,
+
+  lifecycle: lifecycleEnum.singleton,
+});
+
+export default installExtensionInjectable;

src/extensions/extension-installer/install-extensions/install-extensions.injectable.ts

@@ -0,0 +1,30 @@
+/**
+ * Copyright (c) 2021 OpenLens Authors
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy of
+ * this software and associated documentation files (the "Software"), to deal in
+ * the Software without restriction, including without limitation the rights to
+ * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of
+ * the Software, and to permit persons to whom the Software is furnished to do so,
+ * subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
+ * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
+ * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER
+ * IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+import { getInjectable, lifecycleEnum } from "@ogre-tools/injectable";
+import extensionInstallerInjectable from "../extension-installer.injectable";
+
+const installExtensionsInjectable = getInjectable({
+  instantiate: (di) => di.inject(extensionInstallerInjectable).installPackages,
+  
+  lifecycle: lifecycleEnum.singleton,
+});
+
+export default installExtensionsInjectable;