fix(install): reinstall same dependencies (#47)

When installing duplicated dependencies, it triggers concurrency issues
in Node.js (see: nodejs/node#53534).

This can be avoided by pre-deduplication.

Signed-off-by: Kevin Cui <[email protected]>

Author: BlackHole1

src/cmd/install.test.ts

@@ -127,6 +127,9 @@ describe.sequential("install all", () => {
                 publish(path.join(remoteStorage, "c-0.0.2"), ctx.registry.endpoint, "fake-token"),
                 publish(path.join(remoteStorage, "d-0.0.1"), ctx.registry.endpoint, "fake-token"),
                 publish(path.join(remoteStorage, "e-0.0.1"), ctx.registry.endpoint, "fake-token"),
+                publish(path.join(remoteStorage, "e-0.0.2"), ctx.registry.endpoint, "fake-token"),
+                publish(path.join(remoteStorage, "f-0.0.1"), ctx.registry.endpoint, "fake-token"),
+                publish(path.join(remoteStorage, "e-0.0.3"), ctx.registry.endpoint, "fake-token"),
             ]);
         }
 
@@ -155,6 +158,8 @@ describe.sequential("install all", () => {
             "a-0.0.1",
             "b-0.0.1",
             "c-0.0.1",
+            "f-0.0.1",
+            "e-0.0.3",
         ]));
 
         expect(result.deps).toStrictEqual({
@@ -200,6 +205,27 @@ describe.sequential("install all", () => {
                 target: expect.any(String),
                 meta: expect.any(Object),
             },
+            "e-0.0.2": {
+                name: "e",
+                version: "0.0.2",
+                isAlreadyExist: false,
+                target: expect.any(String),
+                meta: expect.any(Object),
+            },
+            "e-0.0.3": {
+                name: "e",
+                version: "0.0.3",
+                isAlreadyExist: false,
+                target: expect.any(String),
+                meta: expect.any(Object),
+            },
+            "f-0.0.1": {
+                name: "f",
+                version: "0.0.1",
+                isAlreadyExist: false,
+                target: expect.any(String),
+                meta: expect.any(Object),
+            },
         });
 
         const fileList = await globby(`**/${ooPackageName}`, {
@@ -215,6 +241,9 @@ describe.sequential("install all", () => {
             "c-0.0.2/package.oo.yaml",
             "d-0.0.1/package.oo.yaml",
             "e-0.0.1/package.oo.yaml",
+            "e-0.0.2/package.oo.yaml",
+            "e-0.0.3/package.oo.yaml",
+            "f-0.0.1/package.oo.yaml",
         ]));
     });
 });

src/utils/fs.test.ts

@@ -0,0 +1,64 @@
+import { readdir } from "node:fs/promises";
+import path from "node:path";
+import { globby } from "globby";
+import { beforeEach, describe, expect, it } from "vitest";
+import { fixture } from "../../tests/helper/fs";
+import { copyDir, remove, tempDir } from "./fs";
+
+beforeEach(async (ctx) => {
+    const temp = await tempDir();
+
+    ctx.workdir = temp;
+
+    return async () => {
+        await remove(temp);
+    };
+});
+
+describe.concurrent("copyDir", () => {
+    it("should copy dir", async (ctx) => {
+        const p = fixture("fs_copy_dir");
+        const target = path.join(ctx.workdir, "target");
+        await copyDir(p, target);
+        expect(await readdir(target)).toEqual(await readdir(p));
+    });
+
+    it("should copy dir with filter", async (ctx) => {
+        const p = fixture("fs_copy_dir");
+        const target = path.join(ctx.workdir, "target");
+
+        const files = await globby("**/3.txt", {
+            cwd: p,
+            dot: true,
+            onlyFiles: false,
+            gitignore: true,
+            absolute: true,
+        });
+
+        await copyDir(p, target, (source, _) => {
+            if (source === p) {
+                return true;
+            }
+
+            return files[0].includes(source);
+        });
+
+        const files2 = await globby("**", {
+            cwd: target,
+            dot: true,
+            onlyFiles: false,
+            gitignore: true,
+            absolute: false,
+        });
+
+        expect(files2).toContain(path.join("bar", "baz", "3.txt"));
+    });
+
+    // see: https://github.com/nodejs/node/pull/53534
+    it.skip("should copy dir with dir already exists", async (ctx) => {
+        const p = fixture("fs_copy_dir");
+        const target = path.join(ctx.workdir, "target");
+        const ps = Array.from({ length: 100 }).fill(0).map(() => copyDir(p, target));
+        await Promise.all(ps);
+    });
+});

src/utils/npm.ts

@@ -127,6 +127,12 @@ export async function transformNodeModules(dir: string) {
         absolute: true,
     });
 
+    const map = new Map<string, {
+        source: string;
+        name: string;
+        version: string;
+    }>();
+
     const ps = result
         // Filter out paths that do not conform to this rule: node_modules/PACKAGE_NAME/package/package.oo.yaml
         .filter((p) => {
@@ -137,12 +143,15 @@ export async function transformNodeModules(dir: string) {
         .map(async (p) => {
             const source = path.dirname(p);
             const { name, version } = await generatePackageJson(source, false);
-            return {
+
+            map.set(`${name}-${version}`, {
                 source,
                 name,
                 version,
-            };
+            });
         });
 
-    return await Promise.all(ps);
+    await Promise.all(ps);
+
+    return Array.from(map.values());
 }

tests/fixtures/fs_copy_dir/1.txt

@@ -6,9 +6,15 @@ entry
     a: 0.0.1
         c: 0.0.2
         d: 0.0.1
+        e: 0.0.2
     b: 0.0.1
         e: 0.0.1
     c: 0.0.1
+        e: 0.0.2
+        b: 0.0.1
+    f: 0.0.1
+        e: 0.0.1
+    e: 0.0.3
 ```
 
 #### Local Storage (Already exists locally)
@@ -26,11 +32,14 @@ b: 0.0.1
 c: 0.0.2
 d: 0.0.1
 e: 0.0.1
+e: 0.0.2
+e: 0.0.3
+f: 0.0.1
 ```
 
 ### Installation Logic
 
-1. Only install the `a:0.0.1` `b:0.0.1` `c:0.0.2` `d:0.0.1` `e:0.0.1` dependencies.
+1. Only install the `a:0.0.1` `b:0.0.1` `c:0.0.2` `d:0.0.1` `e:0.0.1` `e:0.0.2` `f:0.0.1` `e:0.0.3` dependencies.
 2. Do not install the `c:0.0.1` dependency as it is already installed locally.
 3. Do install the `e:0.0.1` dependency as it is sub-dependency of `b:0.0.1`.
    1. _oopm_ don't know about this sub-dependency as it is not mentioned in the `entry` package.

tests/fixtures/fs_copy_dir/bar/2.txt

@@ -4,3 +4,5 @@ dependencies:
   a: 0.0.1
   b: 0.0.1
   c: 0.0.1
+  f: 0.0.1
+  e: 0.0.3

tests/fixtures/fs_copy_dir/bar/baz/3.txt

@@ -3,3 +3,4 @@ version: 0.0.1
 dependencies:
   c: 0.0.2
   d: 0.0.1
+  e: 0.0.2

tests/fixtures/install_all/README.md

@@ -1,2 +1,5 @@
 name: c
 version: 0.0.2
+dependencies:
+  e: 0.0.2
+  b: 0.0.1

tests/fixtures/install_all/entry/package.oo.yaml

@@ -0,0 +1,2 @@
+name: e
+version: 0.0.2

tests/fixtures/install_all/remote_storage/a-0.0.1/package.oo.yaml

@@ -0,0 +1,2 @@
+name: e
+version: 0.0.3

tests/fixtures/install_all/remote_storage/c-0.0.2/package.oo.yaml

@@ -0,0 +1,4 @@
+name: f
+version: 0.0.1
+dependencies:
+  e: 0.0.1