Fix extension install (#7247)

* Fix extension install

- Remove old bundled extension dependencies
- Make sure external extensions are installed as optional

Signed-off-by: Sebastian Malton <sebastian@malton.name>

* Ignore ENOENT errors

Signed-off-by: Sebastian Malton <sebastian@malton.name>

* Add comment

Signed-off-by: Sebastian Malton <sebastian@malton.name>

---------

Signed-off-by: Sebastian Malton <sebastian@malton.name>

packages/core/src/common/fs/fs.injectable.ts

@@ -7,7 +7,7 @@ import type { ReadOptions } from "fs-extra";
 import fse from "fs-extra";
 
 /**
- * NOTE: Add corrisponding a corrisponding override of this injecable in `src/test-utils/override-fs-with-fakes.ts`
+ * NOTE: Add corresponding override of this injectable in `src/test-utils/override-fs-with-fakes.ts`
  */
 const fsInjectable = getInjectable({
   id: "fs",

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

@@ -8,8 +8,8 @@ import extensionLoaderInjectable from "../extension-loader/extension-loader.inje
 import isCompatibleExtensionInjectable from "./is-compatible-extension/is-compatible-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 installExtensionInjectable from "../install-extension/install-extension.injectable";
-import extensionPackageRootDirectoryInjectable from "../extension-installer/extension-package-root-directory/extension-package-root-directory.injectable";
+import extensionPackageRootDirectoryInjectable from "../install-extension/extension-package-root-directory.injectable";
 import readJsonFileInjectable from "../../common/fs/read-json-file.injectable";
 import loggerInjectable from "../../common/logger.injectable";
 import pathExistsInjectable from "../../common/fs/path-exists.injectable";

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

@@ -7,7 +7,7 @@ import type { FSWatcher } from "chokidar";
 import { getDiForUnitTesting } from "../../main/getDiForUnitTesting";
 import extensionDiscoveryInjectable from "../extension-discovery/extension-discovery.injectable";
 import type { ExtensionDiscovery } from "../extension-discovery/extension-discovery";
-import installExtensionInjectable from "../extension-installer/install-extension/install-extension.injectable";
+import installExtensionInjectable from "../install-extension/install-extension.injectable";
 import directoryForUserDataInjectable from "../../common/app-paths/directory-for-user-data/directory-for-user-data.injectable";
 import { delay } from "../../renderer/utils";
 import { observable, runInAction, when } from "mobx";

packages/core/src/extensions/extension-installer/extension-installer.injectable.ts

@@ -1,21 +0,0 @@
-/**
- * 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 pathToNpmCliInjectable from "../../common/app-paths/path-to-npm-cli.injectable";
-import loggerInjectable from "../../common/logger.injectable";
-import { ExtensionInstaller } from "./extension-installer";
-import extensionPackageRootDirectoryInjectable from "./extension-package-root-directory/extension-package-root-directory.injectable";
-
-const extensionInstallerInjectable = getInjectable({
-  id: "extension-installer",
-
-  instantiate: (di) => new ExtensionInstaller({
-    extensionPackageRootDirectory: di.inject(extensionPackageRootDirectoryInjectable),
-    logger: di.inject(loggerInjectable),
-    pathToNpmCli: di.inject(pathToNpmCliInjectable),
-  }),
-});
-
-export default extensionInstallerInjectable;

packages/core/src/extensions/extension-installer/extension-installer.ts

@@ -1,78 +0,0 @@
-/**
- * Copyright (c) OpenLens Authors. All rights reserved.
- * Licensed under MIT License. See LICENSE in root directory for more information.
- */
-
-import AwaitLock from "await-lock";
-import child_process from "child_process";
-import type { Logger } from "../../common/logger";
-
-const logModule = "[EXTENSION-INSTALLER]";
-
-interface Dependencies {
-  readonly extensionPackageRootDirectory: string;
-  readonly logger: Logger;
-  readonly pathToNpmCli: string;
-}
-
-const baseNpmInstallArgs = [
-  "install",
-  "--audit=false",
-  "--fund=false",
-  // NOTE: we do not omit the `optional` dependencies because that is how we specify the non-bundled extensions
-  "--omit=dev",
-  "--omit=peer",
-  "--prefer-offline",
-];
-
-/**
- * Installs dependencies for extensions
- */
-export class ExtensionInstaller {
-  private readonly installLock = new AwaitLock();
-
-  constructor(private readonly dependencies: Dependencies) {}
-
-  /**
-   * Install single package using npm
-   */
-  installPackage = async (name: string): Promise<void> => {
-    // Mutual exclusion to install packages in sequence
-    await this.installLock.acquireAsync();
-
-    try {
-      this.dependencies.logger.info(`${logModule} installing package from ${name} to ${this.dependencies.extensionPackageRootDirectory}`);
-      await this.npm(...baseNpmInstallArgs, name);
-      this.dependencies.logger.info(`${logModule} package ${name} installed to ${this.dependencies.extensionPackageRootDirectory}`);
-    } finally {
-      this.installLock.release();
-    }
-  };
-
-  private npm(...args: string[]): Promise<void> {
-    return new Promise((resolve, reject) => {
-      const child = child_process.fork(this.dependencies.pathToNpmCli, args, {
-        cwd: this.dependencies.extensionPackageRootDirectory,
-        silent: true,
-        env: {},
-      });
-      let stderr = "";
-
-      child.stderr?.on("data", data => {
-        stderr += String(data);
-      });
-
-      child.on("close", (code) => {
-        if (code !== 0) {
-          reject(new Error(stderr));
-        } else {
-          resolve();
-        }
-      });
-
-      child.on("error", error => {
-        reject(error);
-      });
-    });
-  }
-}

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

@@ -1,13 +0,0 @@
-/**
- * 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 extensionInstallerInjectable from "../extension-installer.injectable";
-
-const installExtensionInjectable = getInjectable({
-  id: "install-extension",
-  instantiate: (di) => di.inject(extensionInstallerInjectable).installPackage,
-});
-
-export default installExtensionInjectable;

packages/core/src/extensions/install-extension/extension-package-root-directory.injectable.ts

@@ -3,7 +3,7 @@
  * Licensed under MIT License. See LICENSE in root directory for more information.
  */
 import { getInjectable } from "@ogre-tools/injectable";
-import directoryForUserDataInjectable from "../../../common/app-paths/directory-for-user-data/directory-for-user-data.injectable";
+import directoryForUserDataInjectable from "../../common/app-paths/directory-for-user-data/directory-for-user-data.injectable";
 
 const extensionPackageRootDirectoryInjectable = getInjectable({
   id: "extension-package-root-directory",

packages/core/src/extensions/install-extension/install-extension.injectable.ts

@@ -0,0 +1,111 @@
+/**
+ * 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 { fork } from "child_process";
+import AwaitLock from "await-lock";
+import pathToNpmCliInjectable from "../../common/app-paths/path-to-npm-cli.injectable";
+import extensionPackageRootDirectoryInjectable from "./extension-package-root-directory.injectable";
+import prefixedLoggerInjectable from "../../common/logger/prefixed-logger.injectable";
+import readJsonFileInjectable from "../../common/fs/read-json-file.injectable";
+import joinPathsInjectable from "../../common/path/join-paths.injectable";
+import type { PackageJson } from "../common-api";
+import writeJsonFileInjectable from "../../common/fs/write-json-file.injectable";
+import { once } from "lodash";
+import { isErrnoException } from "../../common/utils";
+
+const baseNpmInstallArgs = [
+  "install",
+  "--save-optional",
+  "--audit=false",
+  "--fund=false",
+  // NOTE: we do not omit the `optional` dependencies because that is how we specify the non-bundled extensions
+  "--omit=dev",
+  "--omit=peer",
+  "--prefer-offline",
+];
+
+export type InstallExtension = (name: string) => Promise<void>;
+
+const installExtensionInjectable = getInjectable({
+  id: "install-extension",
+  instantiate: (di): InstallExtension => {
+    const pathToNpmCli = di.inject(pathToNpmCliInjectable);
+    const extensionPackageRootDirectory = di.inject(extensionPackageRootDirectoryInjectable);
+    const readJsonFile = di.inject(readJsonFileInjectable);
+    const writeJsonFile = di.inject(writeJsonFileInjectable);
+    const joinPaths = di.inject(joinPathsInjectable);
+    const logger = di.inject(prefixedLoggerInjectable, "EXTENSION-INSTALLER");
+
+    const forkNpm = (...args: string[]) => new Promise<void>((resolve, reject) => {
+      const child = fork(pathToNpmCli, args, {
+        cwd: extensionPackageRootDirectory,
+        silent: true,
+        env: {},
+      });
+      let stderr = "";
+
+      child.stderr?.on("data", data => {
+        stderr += String(data);
+      });
+
+      child.on("close", (code) => {
+        if (code !== 0) {
+          reject(new Error(stderr));
+        } else {
+          resolve();
+        }
+      });
+
+      child.on("error", error => {
+        reject(error);
+      });
+    });
+
+    const packageJsonPath = joinPaths(extensionPackageRootDirectory, "package.json");
+
+    /**
+     * NOTES:
+     *   - We have to keep the `package.json` because `npm install` removes files from `node_modules`
+     *     if they are no longer in the `package.json`
+     *   - In v6.2.X we saved bundled extensions as `"dependencies"` and external extensions as
+     *     `"optionalDependencies"` at startup. This was done because `"optionalDependencies"` can
+     *     fail to install and that is OK.
+     *   - We continue to maintain this behavior here by only installing new dependencies as
+     *     `"optionalDependencies"`
+     */
+    const fixupPackageJson = once(async () => {
+      try {
+        const packageJson = await readJsonFile(packageJsonPath) as PackageJson;
+
+        delete packageJson.dependencies;
+
+        await writeJsonFile(packageJsonPath, packageJson);
+      } catch (error) {
+        if (isErrnoException(error) && error.code === "ENOENT") {
+          return;
+        }
+
+        throw error;
+      }
+    });
+
+    const installLock = new AwaitLock();
+
+    return async (name) => {
+      await installLock.acquireAsync();
+      await fixupPackageJson();
+
+      try {
+        logger.info(`installing package for extension "${name}"`);
+        await forkNpm(...baseNpmInstallArgs, name);
+        logger.info(`installed package for extension "${name}"`);
+      } finally {
+        installLock.release();
+      }
+    };
+  },
+});
+
+export default installExtensionInjectable;