fix: empty scoped-registry after remove (#157)

* test: add establishing tests

Add tests to establish the behaviour described in #153.

* feat: manifest prune logic

Add helper function to prune manifest. This removes scoped-registries without scopes.

* feat: prune manifest before saving

Prune manifest before saving to file-system.

src/types/project-manifest.ts

@@ -105,6 +105,25 @@ export function addScopedRegistry(
   manifest.scopedRegistries.push(scopedRegistry);
 }
 
+/**
+ * Removes a scoped-registry to the manifest.
+ * @param manifest The manifest.
+ * @param name The name of the scoped-registry to remove.
+ */
+export function removeScopedRegistry(
+  manifest: UnityProjectManifest,
+  name: string
+) {
+  if (manifest.scopedRegistries === undefined) return;
+
+  const removeIndex = manifest.scopedRegistries.findIndex(
+    (registry) => registry.name === name
+  );
+  if (removeIndex === -1) return;
+
+  manifest.scopedRegistries.splice(removeIndex, 1);
+}
+
 /**
  * Adds a testable to the manifest, if it is not already added.
  * @param manifest The manifest.
@@ -124,3 +143,17 @@ export function addTestable(manifest: UnityProjectManifest, name: DomainName) {
 export function manifestPathFor(projectPath: string): string {
   return path.join(projectPath, "Packages/manifest.json");
 }
+
+/**
+ * Prunes the manifest by performing the following operations:
+ *  - Remove scoped-registries without scopes.
+ * @param manifest The manifest to prune.
+ */
+export function pruneManifest(manifest: UnityProjectManifest) {
+  if (manifest.scopedRegistries !== undefined) {
+    manifest.scopedRegistries.forEach((registry) => {
+      if (registry.scopes.length === 0)
+        removeScopedRegistry(manifest, registry.name);
+    });
+  }
+}

src/utils/project-manifest-io.ts

@@ -3,6 +3,7 @@ import { assertIsError } from "./error-type-guards";
 import log from "../logger";
 import {
   manifestPathFor,
+  pruneManifest,
   UnityProjectManifest,
 } from "../types/project-manifest";
 import fse from "fs-extra";
@@ -34,14 +35,16 @@ export const loadProjectManifest = async function (
 /**
  * Saves a Unity project manifest.
  * @param projectPath The path to the projects root directory.
- * @param data The manifest to save.
+ * @param manifest The manifest to save.
  */
 export const saveProjectManifest = async function (
   projectPath: string,
-  data: UnityProjectManifest
+  manifest: UnityProjectManifest
 ) {
   const manifestPath = manifestPathFor(projectPath);
-  const json = JSON.stringify(data, null, 2);
+  // NOTE: This modifies the manifest that was passed to the function!
+  pruneManifest(manifest);
+  const json = JSON.stringify(manifest, null, 2);
   try {
     await fse.ensureDir(path.dirname(manifestPath));
     await fs.writeFile(manifestPath, json);

test/test-cmd-remove.ts

@@ -12,6 +12,7 @@ import { semanticVersion } from "../src/types/semantic-version";
 import { packageReference } from "../src/types/package-reference";
 import { MockUnityProject, setupUnityProject } from "./setup/unity-project";
 import { after } from "mocha";
+import should from "should";
 
 const packageA = domainName("com.example.package-a");
 const packageB = domainName("com.example.package-b");
@@ -110,5 +111,18 @@ describe("cmd-remove.ts", function () {
         .should.be.ok();
       mockConsole.hasLineIncluding("out", "open Unity").should.be.ok();
     });
+    it("should delete scoped-registry after removing all packages", async () => {
+      const options = {
+        _global: {
+          registry: exampleRegistryUrl,
+        },
+      };
+      const retCode = await remove([packageA, packageB], options);
+      retCode.should.equal(0);
+
+      await mockProject.tryAssertManifest((manifest) => {
+        should(manifest.scopedRegistries).be.empty();
+      });
+    });
   });
 });

test/test-project-manifest-io.ts

@@ -6,13 +6,20 @@ import {
 } from "../src/utils/project-manifest-io";
 import { describe } from "mocha";
 import { shouldNotHaveAnyDependencies } from "./project-manifest-assertions";
-import { domainName } from "../src/types/domain-name";
+import { DomainName, domainName } from "../src/types/domain-name";
 import { semanticVersion } from "../src/types/semantic-version";
-import { addDependency, manifestPathFor } from "../src/types/project-manifest";
+import {
+  addDependency,
+  manifestPathFor,
+  tryGetScopedRegistryByUrl,
+} from "../src/types/project-manifest";
 import should from "should";
 import { MockUnityProject, setupUnityProject } from "./setup/unity-project";
 import fse from "fs-extra";
 import path from "path";
+import { buildProjectManifest } from "./data-project-manifest";
+import { removeScope } from "../src/types/scoped-registry";
+import { exampleRegistryUrl } from "./mock-registry";
 
 describe("project-manifest io", function () {
   let mockConsole: MockConsole = null!;
@@ -67,4 +74,24 @@ describe("project-manifest io", function () {
     const expected = path.join("test-openupm-cli", "Packages", "manifest.json");
     should(manifestPath).be.equal(expected);
   });
+  it("should not save scoped-registry with empty scopes", async () => {
+    // Add and then remove a scope to force an empty scoped-registry
+    const testDomain = "test" as DomainName;
+    const initialManifest = buildProjectManifest((manifest) =>
+      manifest.addScope(testDomain)
+    );
+    const scopedRegistry = tryGetScopedRegistryByUrl(
+      initialManifest,
+      exampleRegistryUrl
+    )!;
+    removeScope(scopedRegistry, testDomain);
+
+    // Save and load manifest
+    (
+      await saveProjectManifest(mockProject.projectPath, initialManifest)
+    ).should.be.ok();
+    const savedManifest = await loadProjectManifest(mockProject.projectPath);
+
+    should(savedManifest?.scopedRegistries).be.empty();
+  });
 });