feat: extract remove service (#352)

* feat: extract remove service This change extracts all business logic out of the remove command into its own service function. The cmd function is now only responsible for cli related logic. This change also slightly changes the behavior of the remove command. - Logs are rewritten - Information about removed packages is printed all at once at the end when the atomic remove process is complete. Previously messages about removed packages were printed as soon as the in-memory manifest was modified. The command could still fail however and then these logs would be confusing. * reformat files
openupm · May 30, 2024 · 61ebed9 · 61ebed9
1 parent 707d97e
commit 61ebed9
Show file tree

Hide file tree

Showing 14 changed files with 320 additions and 243 deletions.
diff --git a/src/cli/cmd-remove.ts b/src/cli/cmd-remove.ts
@@ -1,24 +1,10 @@
 import {
-  LoadProjectManifest,
   ManifestLoadError,
   ManifestWriteError,
-  WriteProjectManifest,
 } from "../io/project-manifest-io";
 import { EnvParseError, ParseEnvService } from "../services/parse-env";
-import {
-  hasVersion,
-  makePackageReference,
-  PackageReference,
-  splitPackageReference,
-} from "../domain/package-reference";
-import { removeScope } from "../domain/scoped-registry";
-import {
-  mapScopedRegistry,
-  removeDependency,
-  UnityProjectManifest,
-} from "../domain/project-manifest";
+import { makePackageReference } from "../domain/package-reference";
 import { CmdOptions } from "./options";
-import { Err, Ok, Result } from "ts-results-es";
 
 import {
   PackageWithVersionError,
@@ -28,9 +14,10 @@ import { Logger } from "npmlog";
 import { ResultCodes } from "./result-codes";
 import {
   notifyEnvParsingFailed,
-  notifyManifestLoadFailed,
-  notifyManifestWriteFailed,
+  notifyPackageRemoveFailed,
 } from "./error-logging";
+import { DomainName } from "../domain/domain-name";
+import { RemovePackages } from "../services/remove-packages";
 
 /**
  * The possible result codes with which the remove command can exit.
@@ -48,11 +35,11 @@ export type RemoveOptions = CmdOptions;
 
 /**
  * Cmd-handler for removing packages.
- * @param pkgs One or multiple package-references to remove.
+ * @param pkgs One or multiple packages to remove.
  * @param options Command options.
  */
 export type RemoveCmd = (
-  pkgs: PackageReference[] | PackageReference,
+  pkgs: ReadonlyArray<DomainName>,
   options: RemoveOptions
 ) => Promise<RemoveResultCode>;
 
@@ -61,12 +48,10 @@ export type RemoveCmd = (
  */
 export function makeRemoveCmd(
   parseEnv: ParseEnvService,
-  loadProjectManifest: LoadProjectManifest,
-  writeProjectManifest: WriteProjectManifest,
+  removePackages: RemovePackages,
   log: Logger
 ): RemoveCmd {
   return async (pkgs, options) => {
-    if (!Array.isArray(pkgs)) pkgs = [pkgs];
     // parse env
     const envResult = await parseEnv(options);
     if (envResult.isErr()) {
@@ -75,59 +60,22 @@ export function makeRemoveCmd(
     }
     const env = envResult.value;
 
-    const tryRemoveFromManifest = async function (
-      manifest: UnityProjectManifest,
-      pkg: PackageReference
-    ): Promise<Result<UnityProjectManifest, RemoveError>> {
-      // parse name
-      if (hasVersion(pkg)) {
-        const [name] = splitPackageReference(pkg);
-        log.warn("", `please do not specify a version (Write only '${name}').`);
-        return Err(new PackageWithVersionError());
-      }
-
-      // not found array
-      const versionInManifest = manifest.dependencies[pkg];
-      if (versionInManifest === undefined) {
-        log.error("404", `package not found: ${pkg}`);
-        return Err(new PackumentNotFoundError());
-      }
-
-      manifest = removeDependency(manifest, pkg);
-
-      manifest = mapScopedRegistry(manifest, env.registry.url, (initial) => {
-        if (initial === null) return null;
-        return removeScope(initial, pkg);
-      });
-
-      log.notice(
-        "manifest",
-        `removed ${makePackageReference(pkg, versionInManifest)}`
-      );
-      return Ok(manifest);
-    };
-
-    // load manifest
-    const manifestResult = await loadProjectManifest(env.cwd).promise;
-    if (manifestResult.isErr()) {
-      notifyManifestLoadFailed(log, manifestResult.error);
+    const removeResult = await removePackages(env.cwd, pkgs).promise;
+    if (removeResult.isErr()) {
+      notifyPackageRemoveFailed(log, removeResult.error);
       return ResultCodes.Error;
     }
-    let manifest = manifestResult.value;
-
-    // remove
-    for (const pkg of pkgs) {
-      const result = await tryRemoveFromManifest(manifest, pkg);
-      if (result.isErr()) return ResultCodes.Error;
-      manifest = result.value;
-    }
+    const removedPackages = removeResult.value;
 
-    // save manifest
-    const saveResult = await writeProjectManifest(env.cwd, manifest).promise;
-    if (saveResult.isErr()) {
-      notifyManifestWriteFailed(log);
-      return ResultCodes.Error;
-    }
+    removedPackages.forEach((removedPackage) => {
+      log.notice(
+        "",
+        `Removed "${makePackageReference(
+          removedPackage.name,
+          removedPackage.version
+        )}".`
+      );
+    });
 
     // print manifest notice
     log.notice("", "please open Unity project to apply changes");

diff --git a/src/cli/error-logging.ts b/src/cli/error-logging.ts
@@ -19,6 +19,7 @@ import { SemanticVersion } from "../domain/semantic-version";
 import { EOL } from "node:os";
 import { ResolveRemotePackumentVersionError } from "../services/resolve-remote-packument-version";
 import { ManifestLoadError } from "../io/project-manifest-io";
+import { RemovePackagesError } from "../services/remove-packages";
 
 export function suggestCheckingWorkingDirectory(log: Logger) {
   log.notice("", "Are you in the correct working directory?");
@@ -182,6 +183,17 @@ export function notifyPackumentNotFoundInAnyRegistry(
   );
 }
 
+export function notifyPackumentNotFoundInManifest(
+  log: Logger,
+  packageName: DomainName
+) {
+  log.error(
+    "",
+    `The package "${packageName}" was not found in your project manifest.`
+  );
+  log.notice("", "Please make sure you have spelled the name correctly.");
+}
+
 export function notifyNoVersions(log: Logger, packageName: DomainName) {
   log.error("", `The package ${packageName} has no versions.`);
 }
@@ -241,3 +253,19 @@ export function notifyRemotePackumentVersionResolvingFailed(
   else if (error instanceof RegistryAuthenticationError)
     notifyRegistryCallFailedBecauseUnauthorized(log);
 }
+
+export function notifyPackageRemoveFailed(
+  log: Logger,
+  error: RemovePackagesError
+) {
+  if (error instanceof FileMissingError) notifyManifestMissing(log, error.path);
+  else if (error instanceof StringFormatError)
+    notifySyntacticallyMalformedProjectManifest(log);
+  else if (error instanceof FileParseError)
+    notifySemanticallyMalformedProjectManifest(log);
+  else if (error instanceof GenericIOError) {
+    if (error.operationType === "Read") notifyManifestLoadFailedBecauseIO(log);
+    else notifyManifestWriteFailed(log);
+  } else if (error instanceof PackumentNotFoundError)
+    notifyPackumentNotFoundInManifest(log, error.packageName);
+}
diff --git a/src/cli/index.ts b/src/cli/index.ts
@@ -48,6 +48,7 @@ import { makeRemotePackumentResolver } from "../services/resolve-remote-packumen
 import { makeLoginService } from "../services/login";
 import { DebugLog } from "../logging";
 import { makeEditorVersionDeterminer } from "../services/determine-editor-version";
+import { makePackageRemover } from "../services/remove-packages";
 
 // Composition root
 
@@ -76,6 +77,10 @@ const fetchPackument = makePackumentFetcher(regClient);
 const fetchAllPackuments = makeAllPackumentsFetcher(debugLog);
 const searchRegistry = makeRegistrySearcher(debugLog);
 const resolveRemotePackument = makeRemotePackumentResolver(fetchPackument);
+const removePackages = makePackageRemover(
+  loadProjectManifest,
+  writeProjectManifest
+);
 
 const parseEnv = makeParseEnvService(
   log,
@@ -118,12 +123,7 @@ const addCmd = makeAddCmd(
 const loginCmd = makeLoginCmd(parseEnv, getUpmConfigPath, login, log);
 const searchCmd = makeSearchCmd(parseEnv, searchPackages, log, debugLog);
 const depsCmd = makeDepsCmd(parseEnv, resolveDependencies, log, debugLog);
-const removeCmd = makeRemoveCmd(
-  parseEnv,
-  loadProjectManifest,
-  writeProjectManifest,
-  log
-);
+const removeCmd = makeRemoveCmd(parseEnv, removePackages, log);
 const viewCmd = makeViewCmd(parseEnv, resolveRemotePackument, log);
 
 // update-notifier
@@ -193,9 +193,9 @@ program
   )
   .aliases(["rm", "uninstall"])
   .description("remove package from manifest json")
-  .action(async function (pkg, otherPkgs, options) {
-    const pkgs = [pkg].concat(otherPkgs);
-    const resultCode = await removeCmd(pkgs, makeCmdOptions(options));
+  .action(async function (packageName, otherPackageNames, options) {
+    const packageNames = [packageName].concat(otherPackageNames);
+    const resultCode = await removeCmd(packageNames, makeCmdOptions(options));
     process.exit(resultCode);
   });
 

diff --git a/src/common-errors.ts b/src/common-errors.ts
@@ -1,13 +1,19 @@
 import { CustomError } from "ts-custom-error";
 import { EditorVersion } from "./domain/editor-version";
+import { DomainName } from "./domain/domain-name";
 
 /**
  * Error for when the packument was not found.
  */
 export class PackumentNotFoundError extends CustomError {
   // noinspection JSUnusedLocalSymbols
   private readonly _class = "PackumentNotFoundError";
-  constructor() {
+  constructor(
+    /**
+     * The name of the missing package.
+     */
+    public packageName: DomainName
+  ) {
     super("A packument was not found.");
   }
 }

diff --git a/src/packument-version-resolving.ts b/src/packument-version-resolving.ts
@@ -62,7 +62,8 @@ export function tryResolveFromCache(
   requestedVersion: ResolvableVersion
 ): Result<ResolvedPackumentVersion, PackumentVersionResolveError> {
   const cachedPackument = tryGetFromCache(cache, source, packumentName);
-  if (cachedPackument === null) return Err(new PackumentNotFoundError());
+  if (cachedPackument === null)
+    return Err(new PackumentNotFoundError(packumentName));
 
   return tryResolvePackumentVersion(cachedPackument, requestedVersion).map(
     (packumentVersion) => ({

diff --git a/src/services/dependency-resolving.ts b/src/services/dependency-resolving.ts
@@ -166,7 +166,7 @@ export function makeResolveDependenciesService(
         let resolveResult: Result<
           ResolvedPackumentVersion,
           ResolveRemotePackumentVersionError
-        > = Err(new PackumentNotFoundError());
+        > = Err(new PackumentNotFoundError(entryName));
         for (const source of sources) {
           const result = await tryResolveFromRegistry(
             source,

diff --git a/src/services/remove-packages.ts b/src/services/remove-packages.ts
@@ -0,0 +1,101 @@
+import { AsyncResult, Err, Ok, Result } from "ts-results-es";
+import {
+  removeDependency,
+  UnityProjectManifest,
+} from "../domain/project-manifest";
+import { PackumentNotFoundError } from "../common-errors";
+import { DomainName } from "../domain/domain-name";
+import {
+  LoadProjectManifest,
+  ManifestLoadError,
+  ManifestWriteError,
+  WriteProjectManifest,
+} from "../io/project-manifest-io";
+import { SemanticVersion } from "../domain/semantic-version";
+import { PackageUrl } from "../domain/package-url";
+
+export type RemovedPackage = {
+  name: DomainName;
+  version: SemanticVersion | PackageUrl;
+};
+
+export type RemovePackagesError =
+  | ManifestLoadError
+  | PackumentNotFoundError
+  | ManifestWriteError;
+
+export type RemovePackages = (
+  projectPath: string,
+  packageNames: ReadonlyArray<DomainName>
+) => AsyncResult<ReadonlyArray<RemovedPackage>, RemovePackagesError>;
+
+export function makePackageRemover(
+  loadProjectManifest: LoadProjectManifest,
+  writeProjectManifest: WriteProjectManifest
+): RemovePackages {
+  const tryRemoveSingle = function (
+    manifest: UnityProjectManifest,
+    packageName: DomainName
+  ): Result<[UnityProjectManifest, RemovedPackage], PackumentNotFoundError> {
+    // not found array
+    const versionInManifest = manifest.dependencies[packageName];
+    if (versionInManifest === undefined) {
+      return Err(new PackumentNotFoundError(packageName));
+    }
+
+    manifest = removeDependency(manifest, packageName);
+
+    manifest = {
+      ...manifest,
+      scopedRegistries: manifest.scopedRegistries
+        // Remove package scope from all scoped registries
+        ?.map((scopedRegistry) => ({
+          ...scopedRegistry,
+          scopes: scopedRegistry.scopes.filter(
+            (scope) => scope !== packageName
+          ),
+        })),
+    };
+
+    return Ok([manifest, { name: packageName, version: versionInManifest }]);
+  };
+
+  function tryRemoveAll(
+    manifest: UnityProjectManifest,
+    packageNames: ReadonlyArray<DomainName>
+  ): Result<
+    [UnityProjectManifest, ReadonlyArray<RemovedPackage>],
+    PackumentNotFoundError
+  > {
+    if (packageNames.length == 0) return Ok([manifest, []]);
+
+    const currentPackageName = packageNames[0]!;
+    const remainingPackageNames = packageNames.slice(1);
+
+    return tryRemoveSingle(manifest, currentPackageName).andThen(
+      ([updatedManifest, removedPackage]) =>
+        tryRemoveAll(updatedManifest, remainingPackageNames).map(
+          ([finalManifest, removedPackages]) => [
+            finalManifest,
+            [removedPackage, ...removedPackages],
+          ]
+        )
+    );
+  }
+
+  return (projectPath, packageNames) => {
+    // load manifest
+    const initialManifest = loadProjectManifest(projectPath);
+
+    // remove
+    const removeResult = initialManifest.andThen((it) =>
+      tryRemoveAll(it, packageNames)
+    );
+
+    return removeResult.andThen(([updatedManifest, removedPackages]) =>
+      writeProjectManifest(projectPath, updatedManifest).map(
+        () => removedPackages
+      )
+    );
+  };
+}
diff --git a/src/services/resolve-latest-version.ts b/src/services/resolve-latest-version.ts
@@ -33,7 +33,7 @@ export function makeResolveLatestVersionService(
     packageName
   ) => {
     if (sources.length === 0)
-      return Err(new PackumentNotFoundError()).toAsyncResult();
+      return Err(new PackumentNotFoundError(packageName)).toAsyncResult();
 
     const sourceToCheck = sources[0]!;
     return fetchPackument(sourceToCheck, packageName).andThen(

diff --git a/src/services/resolve-remote-packument-version.ts b/src/services/resolve-remote-packument-version.ts
@@ -38,7 +38,8 @@ export function makeResolveRemotePackumentVersionService(
   return (packageName, requestedVersion, source) =>
     fetchPackument(source, packageName)
       .andThen((maybePackument) => {
-        if (maybePackument === null) return Err(new PackumentNotFoundError());
+        if (maybePackument === null)
+          return Err(new PackumentNotFoundError(packageName));
         return Ok(maybePackument);
       })
       .andThen((packument) =>