From 2c4a2809738a926604ee154faabd05c04d415c4c Mon Sep 17 00:00:00 2001
From: Ramon Brullo <kingvanti650@gmail.com>
Date: Mon, 15 Apr 2024 15:53:29 +0200
Subject: [PATCH] feat: atomic package add

Currently, when adding multiple packages using the `add` command, each will be added separately. That means, that if packages A, B and C are added and B fails for any reason, A and C will still be added to the manifest.

With this change add operations are now atomic. In the same scenario as before, the manifest would not be modified, since one of the packages could not be added.

A side-effect of this change is that `add` now only returns one error, instead of an array of them, since the first encountered error cancels the operation.

See #223
---
 src/cli/cmd-add.ts   | 17 ++++++-----------
 test/cmd-add.test.ts | 20 ++++++++++++++++++++
 2 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/src/cli/cmd-add.ts b/src/cli/cmd-add.ts
index 9514aa9a..9c81d3bc 100644
--- a/src/cli/cmd-add.ts
+++ b/src/cli/cmd-add.ts
@@ -87,11 +87,11 @@ export type AddError =
 export const add = async function (
   pkgs: PackageReference | PackageReference[],
   options: AddOptions
-): Promise<Result<void, AddError[]>> {
+): Promise<Result<void, AddError>> {
   if (!Array.isArray(pkgs)) pkgs = [pkgs];
   // parse env
   const envResult = await parseEnv(options);
-  if (envResult.isErr()) return Err([envResult.error]);
+  if (envResult.isErr()) return envResult;
   const env = envResult.value;
 
   const client = makeNpmClient();
@@ -299,20 +299,15 @@ export const add = async function (
   const loadResult = await tryLoadProjectManifest(env.cwd).promise;
   if (loadResult.isErr()) {
     logManifestLoadError(loadResult.error);
-    return Err([loadResult.error]);
+    return loadResult;
   }
   let manifest = loadResult.value;
 
   // add
-  const errors = Array.of<AddError>();
   let dirty = false;
-
   for (const pkg of pkgs) {
     const result = await tryAddToManifest(manifest, pkg);
-    if (result.isErr()) {
-      errors.push(result.error);
-      continue;
-    }
+    if (result.isErr()) return result;
 
     const [newManifest, manifestChanged] = result.value;
     if (manifestChanged) {
@@ -326,12 +321,12 @@ export const add = async function (
     const saveResult = await trySaveProjectManifest(env.cwd, manifest).promise;
     if (saveResult.isErr()) {
       logManifestSaveError(saveResult.error);
-      return Err([saveResult.error]);
+      return saveResult;
     }
 
     // print manifest notice
     log.notice("", "please open Unity project to apply changes");
   }
 
-  return errors.length === 0 ? Ok(undefined) : Err(errors);
+  return Ok(undefined);
 };
diff --git a/test/cmd-add.test.ts b/test/cmd-add.test.ts
index 0f8379fc..15e6b4c6 100644
--- a/test/cmd-add.test.ts
+++ b/test/cmd-add.test.ts
@@ -468,5 +468,25 @@ describe("cmd-add.ts", () => {
       expect(addResult).toBeOk();
       expect(warnSpy).toHaveLogLike("package.unity", "2020 is not valid");
     });
+
+    it("should perform multiple adds atomically", async () => {
+      mockResolvedPackuments(
+        [exampleRegistryUrl, remotePackumentA],
+        [exampleRegistryUrl, remotePackumentB]
+      );
+
+      const addResult = await add(
+        // Second package will fail.
+        // Because of this the whole operation should fail.
+        [packageA, packageMissing, packageB],
+        upstreamOptions
+      );
+
+      expect(addResult).toBeError();
+      // Since not all packages could be added, the manifest should not be modified.
+      await mockProject.tryAssertManifest((manifest) =>
+        expect(manifest).toEqual(defaultManifest)
+      );
+    });
   });
 });