Skip to content

Commit ca24297

Browse files
BlackHole1BlackHole1
authored
refactor(scope): use nested directories for scoped packages (#113)
Remove the `+` hack that replaced `/` with `+` in scoped package names. Scoped packages like `@scope/pkg-1.0.0` now use proper nested directory structure (`@scope/pkg-1.0.0`) instead of flattened paths (`@scope+pkg-1.0.0`). Key changes: - Remove `normalizePackageName()` function - Update `move()` to lazily create parent directories on ENOENT - Add comprehensive tests for scoped package handling with nested deps BREAKING CHANGE: Existing distDir with `+` format directories will not be recognized. Users need to reinstall packages. Signed-off-by: Kevin Cui <bh@bugs.cc>
1 parent 1eff4cf commit ca24297

File tree

16 files changed

+194
-28
lines changed

16 files changed

+194
-28
lines changed

‎src/cmd/install.test.ts‎

Lines changed: 60 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -671,16 +671,16 @@ describe.sequential("install deps", () => {
671671
// publish scoped packages to registry
672672
{
673673
const remoteStorage = path.join(p, "remote_storage");
674-
await publish(path.join(remoteStorage, "@foo+bar-1.0.0"), ctx.registry.endpoint, "fake-token");
675-
await publish(path.join(remoteStorage, "@foo+bar-2.0.0"), ctx.registry.endpoint, "fake-token");
676-
await publish(path.join(remoteStorage, "@scope+other-1.0.0"), ctx.registry.endpoint, "fake-token");
674+
await publish(path.join(remoteStorage, "@foo/bar-1.0.0"), ctx.registry.endpoint, "fake-token");
675+
await publish(path.join(remoteStorage, "@foo/bar-2.0.0"), ctx.registry.endpoint, "fake-token");
676+
await publish(path.join(remoteStorage, "@scope/other-1.0.0"), ctx.registry.endpoint, "fake-token");
677677
}
678678

679679
// Copy local_storage to distDir
680680
const distDir = await tempDir();
681681
{
682682
const localStorage = path.join(p, "local_storage");
683-
await copyDir(path.join(localStorage, "@foo+bar-1.0.0"), path.join(distDir, "@foo+bar-1.0.0"));
683+
await copyDir(path.join(localStorage, "@foo/bar-1.0.0"), path.join(distDir, "@foo/bar-1.0.0"));
684684
}
685685

686686
// Copy entry to workdir
@@ -726,9 +726,62 @@ describe.sequential("install deps", () => {
726726
});
727727

728728
expect(new Set(fileList)).toEqual(new Set([
729-
"@foo+bar-1.0.0/package.oo.yaml",
730-
"@foo+bar-2.0.0/package.oo.yaml",
731-
"@scope+other-1.0.0/package.oo.yaml",
729+
"@foo/bar-1.0.0/package.oo.yaml",
730+
"@foo/bar-2.0.0/package.oo.yaml",
731+
"@scope/other-1.0.0/package.oo.yaml",
732+
]));
733+
});
734+
735+
it("should install scoped package with scoped sub-dependencies via integrity check", async (ctx) => {
736+
const p = fixture("scoped_package_with_deps");
737+
738+
const remoteStorage = path.join(p, "remote_storage");
739+
await publish(path.join(remoteStorage, "@other/lib-1.0.0"), ctx.registry.endpoint, "fake-token");
740+
741+
const distDir = await tempDir();
742+
{
743+
const localStorage = path.join(p, "local_storage");
744+
await copyDir(path.join(localStorage, "@scope/pkg-1.0.0"), path.join(distDir, "@scope/pkg-1.0.0"));
745+
}
746+
747+
await copyDir(path.join(p, "entry"), ctx.workdir);
748+
749+
const result = await install({
750+
all: true,
751+
token: "fake-token",
752+
workdir: ctx.workdir,
753+
distDir,
754+
registry: ctx.registry.endpoint,
755+
searchDirs: [path.join(p, "local_storage")],
756+
});
757+
758+
expect(new Set(result.primaryDepNames)).toEqual(new Set([
759+
"@scope/pkg-1.0.0",
760+
]));
761+
762+
expect(result.deps["@scope/pkg-1.0.0"]).toEqual({
763+
name: "@scope/pkg",
764+
version: "1.0.0",
765+
isAlreadyExist: true,
766+
target: expect.any(String),
767+
});
768+
769+
expect(result.deps["@other/lib-1.0.0"]).toEqual({
770+
name: "@other/lib",
771+
version: "1.0.0",
772+
isAlreadyExist: false,
773+
target: expect.any(String),
774+
});
775+
776+
const fileList = await globby(`**/${ooPackageName}`, {
777+
cwd: distDir,
778+
onlyFiles: true,
779+
absolute: false,
780+
});
781+
782+
expect(new Set(fileList)).toEqual(new Set([
783+
"@scope/pkg-1.0.0/package.oo.yaml",
784+
"@other/lib-1.0.0/package.oo.yaml",
732785
]));
733786
});
734787

‎src/cmd/install.ts‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { execa } from "execa";
44
import pLimit from "p-limit";
55
import { ooPackageName, ooThumbnailNames } from "../const";
66
import { copyDir, exists, mkdir, move, remove, tempDir, walk, xTar } from "../utils/fs";
7-
import { env, normalizePackageName } from "../utils/misc";
7+
import { env } from "../utils/misc";
88
import {
99
findLatestVersion,
1010
getDependencies,
@@ -75,7 +75,7 @@ export async function installFile(options: InstallFileOptions): Promise<InstallF
7575
if (isTarball) {
7676
const tempDir = await xTar(options.file);
7777
const { name, version } = await getOOPackageBasicInfo(tempDir);
78-
const targetDir = path.join(options.distDir, `${normalizePackageName(name)}-${version}`);
78+
const targetDir = path.join(options.distDir, `${name}-${version}`);
7979
const isExists = await exists(targetDir);
8080
if (isExists) {
8181
await remove(targetDir);
@@ -92,7 +92,7 @@ export async function installFile(options: InstallFileOptions): Promise<InstallF
9292
}
9393

9494
const { name, version } = await getOOPackageBasicInfo(options.file);
95-
const targetDir = path.join(options.distDir, `${normalizePackageName(name)}-${version}`);
95+
const targetDir = path.join(options.distDir, `${name}-${version}`);
9696
const isExists = await exists(targetDir);
9797
if (isExists) {
9898
await remove(targetDir);
@@ -130,7 +130,7 @@ export async function installPackage(options: InstallPackageOptions): Promise<In
130130

131131
primaryDepNames.push(`${dep.name}-${version}`);
132132

133-
const existsDisk = await exists(path.join(options.distDir, `${normalizePackageName(dep.name)}-${version}`, ooPackageName));
133+
const existsDisk = await exists(path.join(options.distDir, `${dep.name}-${version}`, ooPackageName));
134134
if (existsDisk && dependencies?.[dep.name] === version) {
135135
alreadyInstalled.push({
136136
name: dep.name,
@@ -158,7 +158,7 @@ export async function installPackage(options: InstallPackageOptions): Promise<In
158158
version: dependencies![key],
159159
};
160160

161-
const existsDisk = await exists(path.join(options.distDir, `${normalizePackageName(dep.name)}-${dep.version}`, ooPackageName));
161+
const existsDisk = await exists(path.join(options.distDir, `${dep.name}-${dep.version}`, ooPackageName));
162162
if (!existsDisk) {
163163
needInstall.push(dep);
164164
}
@@ -210,7 +210,7 @@ export async function installAll(options: InstallAllOptions): Promise<InstallAll
210210
version: dependencies![key],
211211
};
212212

213-
const existsDisk = await exists(path.join(options.distDir, `${normalizePackageName(dep.name)}-${dep.version}`, ooPackageName));
213+
const existsDisk = await exists(path.join(options.distDir, `${dep.name}-${dep.version}`, ooPackageName));
214214
if (existsDisk) {
215215
alreadyInstalled.push(dep);
216216
}
@@ -278,7 +278,7 @@ async function _install(options: _InstallOptions): Promise<InstallPackageResult[
278278
const targets: InstallPackageResult["deps"] = {};
279279

280280
for (const dep of options.alreadyInstalled) {
281-
const target = path.join(options.distDir, `${normalizePackageName(dep.name)}-${dep.version}`);
281+
const target = path.join(options.distDir, `${dep.name}-${dep.version}`);
282282

283283
targets[`${dep.name}-${dep.version}`] = {
284284
name: dep.name,
@@ -299,7 +299,7 @@ async function _install(options: _InstallOptions): Promise<InstallPackageResult[
299299
});
300300
})
301301
.map(async (i) => {
302-
const target = path.join(options.distDir, `${normalizePackageName(i.name)}-${i.version}`);
302+
const target = path.join(options.distDir, `${i.name}-${i.version}`);
303303
const isAlreadyExist = await exists(target);
304304

305305
targets[`${i.name}-${i.version}`] = {
@@ -328,7 +328,7 @@ async function _install(options: _InstallOptions): Promise<InstallPackageResult[
328328
const searchDeps = await integrityCheck(options, tempDistDir);
329329
const ps = searchDeps.map(async (dep) => {
330330
const fullName = `${dep.name}-${dep.version}` as const;
331-
const target = path.join(options.distDir, `${normalizePackageName(dep.name)}-${dep.version}`);
331+
const target = path.join(options.distDir, `${dep.name}-${dep.version}`);
332332
if (targets[fullName]) {
333333
return;
334334
}
@@ -397,7 +397,7 @@ async function integrityCheck(options: _InstallOptions, tempDistDir: string): Pr
397397
addedMap.set(`${i.name}-${i.version}`, {
398398
name: i.name,
399399
version: i.version,
400-
distDir: path.join(distDir, `${normalizePackageName(i.name)}-${i.version}`),
400+
distDir: path.join(distDir, `${i.name}-${i.version}`),
401401
});
402402
}
403403
});
@@ -410,7 +410,7 @@ async function integrityCheck(options: _InstallOptions, tempDistDir: string): Pr
410410
const moveLimit = pLimit(10);
411411
const movePs = Array.from(addedMap.values()).map((target) => {
412412
return moveLimit(async () => {
413-
const newTarget = path.join(tempDistDir, `${normalizePackageName(target.name)}-${target.version}`);
413+
const newTarget = path.join(tempDistDir, `${target.name}-${target.version}`);
414414

415415
await move(target.distDir, newTarget);
416416

@@ -456,7 +456,7 @@ async function installMissDep(deps: Deps, options: _InstallOptions): Promise<{
456456
const collectedDeps: Deps = [];
457457

458458
for (const i of info) {
459-
const target = path.join(distDir, `${normalizePackageName(i.name)}-${i.version}`);
459+
const target = path.join(distDir, `${i.name}-${i.version}`);
460460
await move(i.source, target);
461461
collectedDeps.push({
462462
name: i.name,

‎src/cmd/list.test.ts‎

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -120,12 +120,12 @@ describe("list", () => {
120120
{
121121
name: "@foo/bar",
122122
version: "1.0.0",
123-
distDir: path.join(localDir, "@foo+bar-1.0.0"),
123+
distDir: path.join(localDir, "@foo/bar-1.0.0"),
124124
},
125125
{
126126
name: "@scope/other",
127127
version: "1.0.0",
128-
distDir: path.join(localDir2, "@scope+other-1.0.0"),
128+
distDir: path.join(localDir2, "@scope/other-1.0.0"),
129129
},
130130
{
131131
name: "normal-package",
@@ -134,6 +134,36 @@ describe("list", () => {
134134
},
135135
].sort(sortDeps));
136136
});
137+
138+
it("should list scoped packages with nested scoped dependencies", async () => {
139+
const p = fixture("scoped_package_with_deps");
140+
141+
const entryDir = path.join(p, "entry");
142+
const localDir = path.join(p, "local_storage");
143+
const remoteDir = path.join(p, "remote_storage");
144+
145+
const result = await list(entryDir, [localDir, remoteDir]);
146+
147+
expect(result.length).toBe(3);
148+
149+
expect(result.sort(sortDeps)).toStrictEqual([
150+
{
151+
name: "@other/lib",
152+
version: "1.0.0",
153+
distDir: path.join(remoteDir, "@other/lib-1.0.0"),
154+
},
155+
{
156+
name: "@scope/pkg",
157+
version: "1.0.0",
158+
distDir: path.join(localDir, "@scope/pkg-1.0.0"),
159+
},
160+
{
161+
name: "normal-dep",
162+
version: "1.0.0",
163+
distDir: path.join(localDir, "normal-dep-1.0.0"),
164+
},
165+
].sort(sortDeps));
166+
});
137167
});
138168

139169
function sortDeps(a: SearchDep, b: SearchDep) {

‎src/utils/fs.test.ts‎

Lines changed: 75 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
1+
import type { IDepMap } from "../types";
12
import { readdir } from "node:fs/promises";
23
import path from "node:path";
34
import { globby } from "globby";
45
import { beforeEach, describe, expect, it } from "vitest";
56
import { fixture } from "../../tests/helper/fs";
6-
import { copyDir, remove, tempDir } from "./fs";
7+
import { copyDir, exists, mkdir, remove, tempDir, walk } from "./fs";
78

89
beforeEach(async (ctx) => {
910
const temp = await tempDir();
@@ -62,3 +63,76 @@ describe.concurrent("copyDir", () => {
6263
await Promise.all(ps);
6364
});
6465
});
66+
67+
describe("walk", () => {
68+
it("should walk scoped packages with nested dependencies", async () => {
69+
const p = fixture("scoped_package_with_deps");
70+
const localDir = path.join(p, "local_storage");
71+
const remoteDir = path.join(p, "remote_storage");
72+
73+
const map: IDepMap = new Map();
74+
await walk("@scope/pkg", "1.0.0", [localDir, remoteDir], map);
75+
76+
// Should find the scoped package
77+
expect(map.has("@scope/pkg-1.0.0")).toBe(true);
78+
expect(map.get("@scope/pkg-1.0.0")).toEqual({
79+
name: "@scope/pkg",
80+
version: "1.0.0",
81+
distDir: path.join(localDir, "@scope/pkg-1.0.0"),
82+
});
83+
84+
// Should find the normal dependency
85+
expect(map.has("normal-dep-1.0.0")).toBe(true);
86+
expect(map.get("normal-dep-1.0.0")).toEqual({
87+
name: "normal-dep",
88+
version: "1.0.0",
89+
distDir: path.join(localDir, "normal-dep-1.0.0"),
90+
});
91+
92+
// Should find the nested scoped dependency
93+
expect(map.has("@other/lib-1.0.0")).toBe(true);
94+
expect(map.get("@other/lib-1.0.0")).toEqual({
95+
name: "@other/lib",
96+
version: "1.0.0",
97+
distDir: path.join(remoteDir, "@other/lib-1.0.0"),
98+
});
99+
});
100+
101+
it("should handle missing scoped packages", async () => {
102+
const p = fixture("scoped_package_with_deps");
103+
const localDir = path.join(p, "local_storage");
104+
105+
const map: IDepMap = new Map();
106+
await walk("@nonexistent/pkg", "1.0.0", [localDir], map);
107+
108+
expect(map.has("@nonexistent/pkg-1.0.0")).toBe(true);
109+
expect(map.get("@nonexistent/pkg-1.0.0")).toEqual({
110+
name: "@nonexistent/pkg",
111+
version: "1.0.0",
112+
distDir: "",
113+
});
114+
});
115+
});
116+
117+
describe("mkdir with scoped paths", () => {
118+
it("should create nested directories for scoped packages", async (ctx) => {
119+
const scopedPath = path.join(ctx.workdir, "@scope/pkg-1.0.0");
120+
121+
await mkdir(scopedPath);
122+
123+
expect(await exists(scopedPath)).toBe(true);
124+
expect(await exists(path.join(ctx.workdir, "@scope"))).toBe(true);
125+
});
126+
});
127+
128+
describe("exists with scoped paths", () => {
129+
it("should detect scoped package directories", async (ctx) => {
130+
const scopedPath = path.join(ctx.workdir, "@scope/pkg-1.0.0");
131+
132+
expect(await exists(scopedPath)).toBe(false);
133+
134+
await mkdir(scopedPath);
135+
136+
expect(await exists(scopedPath)).toBe(true);
137+
});
138+
});

‎src/utils/fs.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ import fsP from "node:fs/promises";
33
import os from "node:os";
44
import path from "node:path";
55
import * as tar from "tar";
6-
import { normalizePackageName } from "./misc";
76
import { checkOOPackage, getDependencies } from "./npm";
87

98
export async function tempDir() {
@@ -12,6 +11,7 @@ export async function tempDir() {
1211

1312
export async function move(src: string, dest: string) {
1413
try {
14+
await mkdir(path.dirname(dest));
1515
await fsP.rename(src, dest);
1616
}
1717
catch (err) {
@@ -94,7 +94,7 @@ export async function xTar(tarball: string) {
9494
export async function walk(name: string, version: string, searchDirs: string[], map: IDepMap) {
9595
let distDir = "";
9696
for (const searchDir of searchDirs) {
97-
distDir = path.join(searchDir, `${normalizePackageName(name)}-${version}`);
97+
distDir = path.join(searchDir, `${name}-${version}`);
9898

9999
const isCorrect = await checkOOPackage(distDir);
100100
if (isCorrect) {

‎src/utils/misc.ts‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,3 @@ export function nerfURL(url: string) {
1919
const rel = new URL(".", from);
2020
return `//${rel.host}${rel.pathname}`;
2121
}
22-
23-
export function normalizePackageName(name: string): string {
24-
return name.replace(/\//g, "+");
25-
}

tests/fixtures/scoped_package_install/local_storage/@foo+bar-1.0.0/package.oo.yaml renamed to tests/fixtures/scoped_package_install/local_storage/@foo/bar-1.0.0/package.oo.yaml

File renamed without changes.

tests/fixtures/scoped_package_install/remote_storage/@foo+bar-1.0.0/package.oo.yaml renamed to tests/fixtures/scoped_package_install/remote_storage/@foo/bar-1.0.0/package.oo.yaml

File renamed without changes.

tests/fixtures/scoped_package_install/remote_storage/@foo+bar-2.0.0/package.oo.yaml renamed to tests/fixtures/scoped_package_install/remote_storage/@foo/bar-2.0.0/package.oo.yaml

File renamed without changes.

tests/fixtures/scoped_package_install/remote_storage/@scope+other-1.0.0/package.oo.yaml renamed to tests/fixtures/scoped_package_install/remote_storage/@scope/other-1.0.0/package.oo.yaml

File renamed without changes.

0 commit comments

Comments
 (0)