From 5199d909292f4da8a84787d444a74ba8d902b582 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Sat, 31 Aug 2024 10:17:56 +0200 Subject: [PATCH 01/17] refactor: simplify --- src/services/built-in-package-check.ts | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/services/built-in-package-check.ts b/src/services/built-in-package-check.ts index b41d9f82..4250132c 100644 --- a/src/services/built-in-package-check.ts +++ b/src/services/built-in-package-check.ts @@ -1,7 +1,7 @@ import RegClient from "another-npm-registry-client"; import { DomainName } from "../domain/domain-name"; import { packumentHasVersion } from "../domain/packument"; -import { unityRegistryUrl } from "../domain/registry-url"; +import { unityRegistry } from "../domain/registry"; import { SemanticVersion } from "../domain/semantic-version"; import { CheckUrlExists, fetchCheckUrlExists } from "../io/check-url"; import { @@ -34,28 +34,25 @@ export function CheckIsNonRegistryUnityPackage( checkUrlExists: CheckUrlExists, getRegistryPackument: GetRegistryPackument ): CheckIsBuiltInPackage { - function checkIsUnityPackage(packageName: DomainName) { + function hasDocPage(packageName: DomainName) { // A package is an official Unity package if it has a documentation page const url = docUrlForPackage(packageName); return checkUrlExists(url); } - async function checkExistsOnUnityRegistry( + async function versionIsOnRegistry( packageName: DomainName, version: SemanticVersion ): Promise { - const packument = await getRegistryPackument( - { url: unityRegistryUrl, auth: null }, - packageName - ); - if (packument === null) return false; - return packumentHasVersion(packument, version); + const packument = await getRegistryPackument(unityRegistry, packageName); + return packument !== null && packumentHasVersion(packument, version); } return async (packageName, version) => { - const isUnityPackage = await checkIsUnityPackage(packageName); + const isUnityPackage = await hasDocPage(packageName); if (!isUnityPackage) return false; - return !(await checkExistsOnUnityRegistry(packageName, version)); + const isOnUnityRegistry = await versionIsOnRegistry(packageName, version); + return !isOnUnityRegistry; }; } From 277756ec75196ddf5eabcb12cb1efcd28def9603 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Sun, 1 Sep 2024 08:11:09 +0200 Subject: [PATCH 02/17] refactor: extract pure --- src/services/search-packages.ts | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/src/services/search-packages.ts b/src/services/search-packages.ts index f2e73ae4..10df049f 100644 --- a/src/services/search-packages.ts +++ b/src/services/search-packages.ts @@ -2,6 +2,7 @@ import { Registry } from "../domain/registry"; import { GetAllRegistryPackuments, getAllRegistryPackumentsUsing, + type AllPackuments, } from "../io/all-packuments-io"; import { SearchedPackument, @@ -24,6 +25,19 @@ export type SearchPackages = ( onUseAllFallback?: () => void ) => Promise>; +function tryFindPackagesByKeyword( + allPackuments: AllPackuments, + keyword: string +): ReadonlyArray { + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const { _updated, ...packumentEntries } = allPackuments; + const packuments = Object.values(packumentEntries); + const klc = keyword.toLowerCase(); + return packuments.filter((packument) => + packument.name.toLowerCase().includes(klc) + ); +} + /** * Makes a {@licence SearchPackages} function which searches a registry both * using the search endpoint and, as a fallback, by querying all packages @@ -34,20 +48,6 @@ export function ApiAndFallbackPackageSearch( getAllRegistryPackuments: GetAllRegistryPackuments, debugLog: DebugLog ): SearchPackages { - async function searchInAll( - registry: Registry, - keyword: string - ): Promise { - const allPackuments = await getAllRegistryPackuments(registry); - // eslint-disable-next-line @typescript-eslint/no-unused-vars - const { _updated, ...packumentEntries } = allPackuments; - const packuments = Object.values(packumentEntries); - const klc = keyword.toLowerCase(); - return packuments.filter((packument) => - packument.name.toLowerCase().includes(klc) - ); - } - return async (registry, keyword, onUseOldSearch) => { try { // search endpoint @@ -57,7 +57,8 @@ export function ApiAndFallbackPackageSearch( debugLog("Searching using search endpoint failed", error); // search old search onUseOldSearch && onUseOldSearch(); - return await searchInAll(registry, keyword); + const allPackuments = await getAllRegistryPackuments(registry); + return tryFindPackagesByKeyword(allPackuments, keyword); } }; } From b3928eaa55ddbbb5317c8c7804aec5a2eb142a24 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Sun, 1 Sep 2024 08:39:00 +0200 Subject: [PATCH 03/17] refactor: extract pure --- src/services/remove-packages.ts | 15 +++++---------- src/utils/object-utils.ts | 16 ++++++++++++++++ test/unit/utils/object-utils.test.ts | 21 +++++++++++++++++++++ 3 files changed, 42 insertions(+), 10 deletions(-) create mode 100644 src/utils/object-utils.ts create mode 100644 test/unit/utils/object-utils.test.ts diff --git a/src/services/remove-packages.ts b/src/services/remove-packages.ts index 39b2ecd2..b47bd3a6 100644 --- a/src/services/remove-packages.ts +++ b/src/services/remove-packages.ts @@ -14,6 +14,7 @@ import { SaveProjectManifest, } from "../io/project-manifest-io"; import { DebugLog } from "../logging"; +import { omitKey } from "../utils/object-utils"; import { resultifyAsyncOp } from "../utils/result-utils"; /** @@ -84,11 +85,8 @@ export function RemovePackagesFromManifest( }; // Remove scoped registries property if empty - if (manifest.scopedRegistries!.length === 0) { - // eslint-disable-next-line @typescript-eslint/no-unused-vars - const { scopedRegistries, ...withoutScopedRegistries } = manifest; - manifest = withoutScopedRegistries; - } + if (manifest.scopedRegistries!.length === 0) + manifest = omitKey(manifest, "scopedRegistries"); } if (manifest.testables !== undefined) { @@ -98,11 +96,8 @@ export function RemovePackagesFromManifest( }; // Remove testables property if empty - if (manifest.testables!.length === 0) { - // eslint-disable-next-line @typescript-eslint/no-unused-vars - const { testables, ...withoutTestables } = manifest; - manifest = withoutTestables; - } + if (manifest.testables!.length === 0) + manifest = omitKey(manifest, "testables"); } return Ok([manifest, { name: packageName, version: versionInManifest }]); diff --git a/src/utils/object-utils.ts b/src/utils/object-utils.ts new file mode 100644 index 00000000..dcb4455b --- /dev/null +++ b/src/utils/object-utils.ts @@ -0,0 +1,16 @@ +/** + * Deletes a property from an object in an immutable way. The original object is + * not modified. Instead a shallow copy of the object, without the key is + * created. + * @param obj The original object. + * @param key The key to remove. + * @returns A copy of the object without the key. + */ +export function omitKey( + obj: T, + key: TKey +): Omit { + const copy = { ...obj }; + delete copy[key]; + return copy; +} diff --git a/test/unit/utils/object-utils.test.ts b/test/unit/utils/object-utils.test.ts new file mode 100644 index 00000000..c945790f --- /dev/null +++ b/test/unit/utils/object-utils.test.ts @@ -0,0 +1,21 @@ +import { omitKey } from "../../../src/utils/object-utils"; + +describe("object utils", () => { + describe("omit key", () => { + it("should omit key", () => { + const original = { a: 1, b: 2 }; + + const actual = omitKey(original, "a"); + + expect(actual).toEqual({ b: 2 }); + }); + + it("should not modify original", () => { + const original = { a: 1, b: 2 }; + + omitKey(original, "a"); + + expect(original).toEqual({ a: 1, b: 2 }); + }); + }); +}); From c7e6612c5195ca573a4fbcb9dd712001ea18ab4c Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Sun, 1 Sep 2024 08:50:26 +0200 Subject: [PATCH 04/17] refactor: extract pure --- src/domain/project-manifest.ts | 29 +++++++-- src/services/remove-packages.ts | 13 +---- test/unit/domain/project-manifest.test.ts | 71 +++++++++++++++++++++-- 3 files changed, 92 insertions(+), 21 deletions(-) diff --git a/src/domain/project-manifest.ts b/src/domain/project-manifest.ts index aab48c18..2551be56 100644 --- a/src/domain/project-manifest.ts +++ b/src/domain/project-manifest.ts @@ -1,10 +1,10 @@ +import { removeRecordKey } from "../utils/record-utils"; +import { removeTrailingSlash } from "../utils/string-utils"; import { DomainName } from "./domain-name"; -import { SemanticVersion } from "./semantic-version"; import { PackageUrl } from "./package-url"; -import { ScopedRegistry } from "./scoped-registry"; import { RegistryUrl } from "./registry-url"; -import { removeTrailingSlash } from "../utils/string-utils"; -import { removeRecordKey } from "../utils/record-utils"; +import { ScopedRegistry } from "./scoped-registry"; +import { SemanticVersion } from "./semantic-version"; /** * The content of the project-manifest (manifest.json) of a Unity project. @@ -157,3 +157,24 @@ export function hasDependency( ): boolean { return packageName in manifest.dependencies; } + +/** + * Removes a package name from all scope lists of all scoped registries + * in a manifest. + * @param manifest The manifest. + * @param packageName The package name. + * @returns The manifest with the scope removed. + */ +export function removeScopeFromAllScopedRegistries( + manifest: UnityProjectManifest, + packageName: DomainName +): UnityProjectManifest { + if (manifest.scopedRegistries === undefined) return manifest; + return { + ...manifest, + scopedRegistries: manifest.scopedRegistries.map((scopedRegistry) => ({ + ...scopedRegistry, + scopes: scopedRegistry.scopes.filter((scope) => scope !== packageName), + })), + }; +} diff --git a/src/services/remove-packages.ts b/src/services/remove-packages.ts index b47bd3a6..813f603e 100644 --- a/src/services/remove-packages.ts +++ b/src/services/remove-packages.ts @@ -4,6 +4,7 @@ import { DomainName } from "../domain/domain-name"; import { PackageUrl } from "../domain/package-url"; import { removeDependency, + removeScopeFromAllScopedRegistries, UnityProjectManifest, } from "../domain/project-manifest"; import { SemanticVersion } from "../domain/semantic-version"; @@ -64,17 +65,6 @@ export function RemovePackagesFromManifest( manifest = removeDependency(manifest, packageName); if (manifest.scopedRegistries !== undefined) { - manifest = { - ...manifest, - scopedRegistries: manifest.scopedRegistries - // Remove package scope from all scoped registries - .map((scopedRegistry) => ({ - ...scopedRegistry, - scopes: scopedRegistry.scopes.filter( - (scope) => scope !== packageName - ), - })), - }; // Remove scoped registries without scopes manifest = { @@ -83,6 +73,7 @@ export function RemovePackagesFromManifest( (it) => it.scopes.length > 0 ), }; + manifest = removeScopeFromAllScopedRegistries(manifest, packageName); // Remove scoped registries property if empty if (manifest.scopedRegistries!.length === 0) diff --git a/test/unit/domain/project-manifest.test.ts b/test/unit/domain/project-manifest.test.ts index ac4f22b3..4cd2d7f0 100644 --- a/test/unit/domain/project-manifest.test.ts +++ b/test/unit/domain/project-manifest.test.ts @@ -1,21 +1,26 @@ +import fc from "fast-check"; +import { DomainName } from "../../../src/domain/domain-name"; import { addTestable, emptyProjectManifest, hasDependency, mapScopedRegistry, removeDependency, + removeScopeFromAllScopedRegistries, setDependency, setScopedRegistry, tryGetScopedRegistryByUrl, + type UnityProjectManifest, } from "../../../src/domain/project-manifest"; -import { DomainName } from "../../../src/domain/domain-name"; +import { RegistryUrl } from "../../../src/domain/registry-url"; +import { + addScope, + makeScopedRegistry, +} from "../../../src/domain/scoped-registry"; import { SemanticVersion } from "../../../src/domain/semantic-version"; -import { addScope, makeScopedRegistry } from "../../../src/domain/scoped-registry"; -import fc from "fast-check"; -import { arbDomainName } from "./domain-name.arb"; -import { exampleRegistryUrl } from "./data-registry"; import { buildProjectManifest } from "./data-project-manifest"; -import { RegistryUrl } from "../../../src/domain/registry-url"; +import { exampleRegistryUrl } from "./data-registry"; +import { arbDomainName } from "./domain-name.arb"; describe("project-manifest", () => { describe("set dependency", () => { @@ -204,4 +209,58 @@ describe("project-manifest", () => { expect(hasDependency(emptyProjectManifest, packageName)).toBeFalsy(); }); }); + + describe("remove scope from all scoped registries", () => { + it('should have no effect for undefined "scopedRegistries"', () => { + fc.assert( + fc.property(arbDomainName, (scope) => { + const original: UnityProjectManifest = { dependencies: {} }; + + const actual = removeScopeFromAllScopedRegistries(original, scope); + + expect(actual).toEqual(original); + }) + ); + }); + + it("should remove scope from all scoped registries", () => { + fc.assert( + fc.property(arbDomainName, arbDomainName, (scope1, scope2) => { + const original: UnityProjectManifest = { + dependencies: {}, + scopedRegistries: [ + { + name: "Scope A", + url: RegistryUrl.parse("https://a.com"), + scopes: [scope1], + }, + { + name: "Scope B", + url: RegistryUrl.parse("https://b.com"), + scopes: [scope1, scope2], + }, + ], + }; + + const actual = removeScopeFromAllScopedRegistries(original, scope1); + + expect(actual).toEqual({ + dependencies: {}, + scopedRegistries: [ + { + name: "Scope A", + url: RegistryUrl.parse("https://a.com"), + scopes: [], + }, + { + name: "Scope B", + url: RegistryUrl.parse("https://b.com"), + scopes: [scope2], + }, + ], + }); + }) + ); + }); + }); }); From fb924144aa5ba201e1c6d272e466743d890db561 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Sun, 1 Sep 2024 09:04:10 +0200 Subject: [PATCH 05/17] refactor: extract pure --- src/domain/project-manifest.ts | 18 ++++++++ src/services/remove-packages.ts | 10 +---- test/unit/domain/project-manifest.test.ts | 52 +++++++++++++++++++++++ 3 files changed, 72 insertions(+), 8 deletions(-) diff --git a/src/domain/project-manifest.ts b/src/domain/project-manifest.ts index 2551be56..6980ff92 100644 --- a/src/domain/project-manifest.ts +++ b/src/domain/project-manifest.ts @@ -178,3 +178,21 @@ export function removeScopeFromAllScopedRegistries( })), }; } + +/** + * Removes all empty scoped registries from a manifest. A scoped registry + * is empty, if it has no scopes. + * @param manifest The manifest. + * @returns The manifest without empty scoped registried. + */ +export function removeEmptyScopedRegistries( + manifest: UnityProjectManifest +): UnityProjectManifest { + if (manifest.scopedRegistries === undefined) return manifest; + return { + ...manifest, + scopedRegistries: manifest.scopedRegistries.filter( + (it) => it.scopes.length > 0 + ), + }; +} diff --git a/src/services/remove-packages.ts b/src/services/remove-packages.ts index 813f603e..7e1801e3 100644 --- a/src/services/remove-packages.ts +++ b/src/services/remove-packages.ts @@ -4,6 +4,7 @@ import { DomainName } from "../domain/domain-name"; import { PackageUrl } from "../domain/package-url"; import { removeDependency, + removeEmptyScopedRegistries, removeScopeFromAllScopedRegistries, UnityProjectManifest, } from "../domain/project-manifest"; @@ -65,15 +66,8 @@ export function RemovePackagesFromManifest( manifest = removeDependency(manifest, packageName); if (manifest.scopedRegistries !== undefined) { - - // Remove scoped registries without scopes - manifest = { - ...manifest, - scopedRegistries: manifest.scopedRegistries!.filter( - (it) => it.scopes.length > 0 - ), - }; manifest = removeScopeFromAllScopedRegistries(manifest, packageName); + manifest = removeEmptyScopedRegistries(manifest); // Remove scoped registries property if empty if (manifest.scopedRegistries!.length === 0) diff --git a/test/unit/domain/project-manifest.test.ts b/test/unit/domain/project-manifest.test.ts index 4cd2d7f0..cde5bdb6 100644 --- a/test/unit/domain/project-manifest.test.ts +++ b/test/unit/domain/project-manifest.test.ts @@ -6,6 +6,7 @@ import { hasDependency, mapScopedRegistry, removeDependency, + removeEmptyScopedRegistries, removeScopeFromAllScopedRegistries, setDependency, setScopedRegistry, @@ -263,4 +264,55 @@ describe("project-manifest", () => { ); }); }); + + describe("remove empty scoped registries", () => { + it('should have no effect for undefined "scopedRegistries"', () => { + const original: UnityProjectManifest = { dependencies: {} }; + + const actual = removeEmptyScopedRegistries(original); + + expect(actual).toEqual(original); + }); + + it("should leave non-empty scoped registries", () => { + fc.assert( + fc.property(arbDomainName, (scope) => { + const original: UnityProjectManifest = { + dependencies: {}, + scopedRegistries: [ + { + name: "Scope A", + url: RegistryUrl.parse("https://a.com"), + scopes: [scope], + }, + ], + }; + + const actual = removeEmptyScopedRegistries(original); + + expect(actual).toEqual(original); + }) + ); + }); + + it("should remove empty scoped registries", () => { + const original: UnityProjectManifest = { + dependencies: {}, + scopedRegistries: [ + { + name: "Scope A", + url: RegistryUrl.parse("https://a.com"), + scopes: [], + }, + ], + }; + + const actual = removeEmptyScopedRegistries(original); + + expect(actual).toEqual({ + dependencies: {}, + scopedRegistries: [], + }); + }); + }); }); From 883d68b017f28fa0147350b78453344095dee940 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Sun, 1 Sep 2024 09:10:26 +0200 Subject: [PATCH 06/17] refactor: extract pure --- src/domain/project-manifest.ts | 17 ++++++++++++ src/services/remove-packages.ts | 31 ++++++++------------- test/unit/domain/project-manifest.test.ts | 33 +++++++++++++++++++++++ 3 files changed, 61 insertions(+), 20 deletions(-) diff --git a/src/domain/project-manifest.ts b/src/domain/project-manifest.ts index 6980ff92..3f01004d 100644 --- a/src/domain/project-manifest.ts +++ b/src/domain/project-manifest.ts @@ -196,3 +196,20 @@ export function removeEmptyScopedRegistries( ), }; } + +/** + * Removes a testable from this manifest. + * @param manifest The manifest. + * @param packageName The name of the testable to remove. + * @returns The manifest without the testable. + */ +export function removeTestable( + manifest: UnityProjectManifest, + packageName: DomainName +): UnityProjectManifest { + if (manifest.testables === undefined) return manifest; + return { + ...manifest, + testables: manifest.testables.filter((it) => it !== packageName), + }; +} diff --git a/src/services/remove-packages.ts b/src/services/remove-packages.ts index 7e1801e3..a994d3a9 100644 --- a/src/services/remove-packages.ts +++ b/src/services/remove-packages.ts @@ -6,6 +6,7 @@ import { removeDependency, removeEmptyScopedRegistries, removeScopeFromAllScopedRegistries, + removeTestable, UnityProjectManifest, } from "../domain/project-manifest"; import { SemanticVersion } from "../domain/semantic-version"; @@ -64,26 +65,16 @@ export function RemovePackagesFromManifest( return Err(new PackumentNotFoundError(packageName)); manifest = removeDependency(manifest, packageName); - - if (manifest.scopedRegistries !== undefined) { - manifest = removeScopeFromAllScopedRegistries(manifest, packageName); - manifest = removeEmptyScopedRegistries(manifest); - - // Remove scoped registries property if empty - if (manifest.scopedRegistries!.length === 0) - manifest = omitKey(manifest, "scopedRegistries"); - } - - if (manifest.testables !== undefined) { - manifest = { - ...manifest, - testables: manifest.testables.filter((it) => it !== packageName), - }; - - // Remove testables property if empty - if (manifest.testables!.length === 0) - manifest = omitKey(manifest, "testables"); - } + manifest = removeScopeFromAllScopedRegistries(manifest, packageName); + manifest = removeEmptyScopedRegistries(manifest); + manifest = removeTestable(manifest, packageName); + + // Remove scoped registries property if empty + if (manifest.scopedRegistries?.length === 0) + manifest = omitKey(manifest, "scopedRegistries"); + // Remove testables property if empty + if (manifest.testables?.length === 0) + manifest = omitKey(manifest, "testables"); return Ok([manifest, { name: packageName, version: versionInManifest }]); }; diff --git a/test/unit/domain/project-manifest.test.ts b/test/unit/domain/project-manifest.test.ts index cde5bdb6..754e882b 100644 --- a/test/unit/domain/project-manifest.test.ts +++ b/test/unit/domain/project-manifest.test.ts @@ -8,6 +8,7 @@ import { removeDependency, removeEmptyScopedRegistries, removeScopeFromAllScopedRegistries, + removeTestable, setDependency, setScopedRegistry, tryGetScopedRegistryByUrl, @@ -315,4 +316,36 @@ describe("project-manifest", () => { }); }); }); + + describe("remove testable", () => { + it('should have no effect for undefined "testables"', () => { + fc.assert( + fc.property(arbDomainName, (packageName) => { + const original: UnityProjectManifest = { dependencies: {} }; + + const actual = removeTestable(original, packageName); + + expect(actual).toEqual(original); + }) + ); + }); + + it("should remove testable", () => { + fc.assert( + fc.property(arbDomainName, (packageName) => { + const original: UnityProjectManifest = { + dependencies: {}, + testables: [packageName], + }; + + const actual = removeTestable(original, packageName); + + expect(actual).toEqual({ + dependencies: {}, + testables: [], + }); + }) + ); + }); + }); }); From 4d4d40ddc6dcd38ea5dbd8bf1ef8542badff4224 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Sun, 1 Sep 2024 10:05:49 +0200 Subject: [PATCH 07/17] test: add custom arbitraries --- test/unit/domain/project-manifest.arb.ts | 53 ++++++++++++++++++++++++ test/unit/domain/semantic-version.arb.ts | 13 ++++++ 2 files changed, 66 insertions(+) create mode 100644 test/unit/domain/project-manifest.arb.ts create mode 100644 test/unit/domain/semantic-version.arb.ts diff --git a/test/unit/domain/project-manifest.arb.ts b/test/unit/domain/project-manifest.arb.ts new file mode 100644 index 00000000..cc44f0c2 --- /dev/null +++ b/test/unit/domain/project-manifest.arb.ts @@ -0,0 +1,53 @@ +import fc, { type Arbitrary } from "fast-check"; +import { type UnityProjectManifest } from "../../../src/domain/project-manifest"; +import { UnityProjectManifestBuilder } from "./data-project-manifest"; +import { arbDomainName } from "./domain-name.arb"; +import { arbSemanticVersion } from "./semantic-version.arb"; + +function withArbDependencies( + builder: UnityProjectManifestBuilder, + dependencyCount: number +): Arbitrary { + if (dependencyCount === 0) return fc.constant(builder.manifest); + + return fc + .tuple(arbDomainName, arbSemanticVersion, fc.boolean()) + .chain(([packageName, version, withTestable]) => { + const withDependency = builder.addDependency( + packageName, + version, + true, + withTestable + ); + return withArbDependencies(withDependency, dependencyCount - 1); + }); +} + +/** + * Makes an arbitrary for a {@link UnityProjectManifest} with a specific + * number of dependencies. + * @param dependencyCount The nuber of dependencies. + * @returns The arbitrary. + */ +export function arbManifestWithDependencyCount( + dependencyCount: number +): Arbitrary { + return withArbDependencies( + new UnityProjectManifestBuilder(), + dependencyCount + ); +} + +/** + * Arbitrary {@link UnityProjectManifest} with 0 or more dependencies. + */ +export const arbManifest = fc + .integer({ min: 0, max: 20 }) + .chain(arbManifestWithDependencyCount); + +/** + * Arbitrary {@link UnityProjectManifest} with at least 1 dependency. + */ +export const arbNonEmptyManifest = fc + .integer({ min: 1, max: 20 }) + .chain(arbManifestWithDependencyCount); diff --git a/test/unit/domain/semantic-version.arb.ts b/test/unit/domain/semantic-version.arb.ts new file mode 100644 index 00000000..da6e51f5 --- /dev/null +++ b/test/unit/domain/semantic-version.arb.ts @@ -0,0 +1,13 @@ +import fc from "fast-check"; +import { SemanticVersion } from "../../../src/domain/semantic-version"; + +const segment = fc.integer({ min: 0, max: 10 }); + +/** + * An arbitrary {@link SemanticVersion} to be used in property tests. + */ +export const arbSemanticVersion = fc + .tuple(segment, segment, segment) + .map(([major, minor, patch]) => + SemanticVersion.parse(`${major}.${minor}.${patch}`) + ); From a2736a97d64dd4d93021b47272ddf449ae3c3665 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Sun, 1 Sep 2024 10:45:04 +0200 Subject: [PATCH 08/17] refactor: make builder immutable This makes it easier to use in arbitraries --- test/unit/domain/data-project-manifest.ts | 42 +++++++++++------------ test/unit/domain/project-manifest.arb.ts | 2 +- 2 files changed, 21 insertions(+), 23 deletions(-) diff --git a/test/unit/domain/data-project-manifest.ts b/test/unit/domain/data-project-manifest.ts index 1dcdd1e7..1eaf072e 100644 --- a/test/unit/domain/data-project-manifest.ts +++ b/test/unit/domain/data-project-manifest.ts @@ -1,5 +1,8 @@ import { SemanticVersion } from "../../../src/domain/semantic-version"; -import { addScope, makeScopedRegistry } from "../../../src/domain/scoped-registry"; +import { + addScope, + makeScopedRegistry, +} from "../../../src/domain/scoped-registry"; import { addTestable, emptyProjectManifest, @@ -14,12 +17,10 @@ import { DomainName } from "../../../src/domain/domain-name"; /** * Builder class for {@link UnityProjectManifest}. */ -class UnityProjectManifestBuilder { - manifest: UnityProjectManifest; +export class UnityProjectManifestBuilder { + private constructor(public readonly manifest: UnityProjectManifest) {} - constructor() { - this.manifest = emptyProjectManifest; - } + public static empty = new UnityProjectManifestBuilder(emptyProjectManifest); /** * Add a scope to the manifests scoped registry. @@ -28,17 +29,13 @@ class UnityProjectManifestBuilder { addScope(name: string): UnityProjectManifestBuilder { assertZod(name, DomainName); - this.manifest = mapScopedRegistry( - this.manifest, - exampleRegistryUrl, - (registry) => { + return new UnityProjectManifestBuilder( + mapScopedRegistry(this.manifest, exampleRegistryUrl, (registry) => { if (registry === null) registry = makeScopedRegistry("example.com", exampleRegistryUrl); return addScope(registry, name); - } + }) ); - - return this; } /** @@ -47,8 +44,7 @@ class UnityProjectManifestBuilder { */ addTestable(name: string): UnityProjectManifestBuilder { assertZod(name, DomainName); - this.manifest = addTestable(this.manifest, name); - return this; + return new UnityProjectManifestBuilder(addTestable(this.manifest, name)); } /** @@ -66,10 +62,12 @@ class UnityProjectManifestBuilder { ): UnityProjectManifestBuilder { assertZod(name, DomainName); assertZod(version, SemanticVersion); - if (withScope) this.addScope(name); - if (testable) this.addTestable(name); - this.manifest = setDependency(this.manifest, name, version); - return this; + let updated: UnityProjectManifestBuilder = new UnityProjectManifestBuilder( + setDependency(this.manifest, name, version) + ); + if (withScope) updated = updated.addScope(name); + if (testable) updated = updated.addTestable(name); + return updated; } } @@ -79,9 +77,9 @@ class UnityProjectManifestBuilder { * @param build A builder function. */ export function buildProjectManifest( - build?: (builder: UnityProjectManifestBuilder) => unknown + build?: (builder: UnityProjectManifestBuilder) => UnityProjectManifestBuilder ) { - const builder = new UnityProjectManifestBuilder(); - if (build !== undefined) build(builder); + let builder = UnityProjectManifestBuilder.empty; + if (build !== undefined) builder = build(builder); return builder.manifest; } diff --git a/test/unit/domain/project-manifest.arb.ts b/test/unit/domain/project-manifest.arb.ts index cc44f0c2..ec1c2359 100644 --- a/test/unit/domain/project-manifest.arb.ts +++ b/test/unit/domain/project-manifest.arb.ts @@ -33,7 +33,7 @@ export function arbManifestWithDependencyCount( dependencyCount: number ): Arbitrary { return withArbDependencies( - new UnityProjectManifestBuilder(), + UnityProjectManifestBuilder.empty, dependencyCount ); } From 24bb07b475cdc020b26a8bd3b2def67e9be4581f Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Sun, 1 Sep 2024 11:13:34 +0200 Subject: [PATCH 09/17] refactor: extract pure logic --- src/domain/dependency-management.ts | 59 ++++++ src/services/remove-packages.ts | 38 +--- .../unit/domain/dependency-management.test.ts | 168 ++++++++++++++++++ test/unit/services/remove-packages.test.ts | 6 +- 4 files changed, 233 insertions(+), 38 deletions(-) create mode 100644 src/domain/dependency-management.ts create mode 100644 test/unit/domain/dependency-management.test.ts diff --git a/src/domain/dependency-management.ts b/src/domain/dependency-management.ts new file mode 100644 index 00000000..57aae55c --- /dev/null +++ b/src/domain/dependency-management.ts @@ -0,0 +1,59 @@ +import { Err, Ok, type Result } from "ts-results-es"; +import { PackumentNotFoundError } from "../common-errors"; +import { omitKey } from "../utils/object-utils"; +import type { DomainName } from "./domain-name"; +import type { PackageUrl } from "./package-url"; +import { + type UnityProjectManifest, + removeDependency, + removeEmptyScopedRegistries, + removeScopeFromAllScopedRegistries, + removeTestable, +} from "./project-manifest"; +import type { SemanticVersion } from "./semantic-version"; + +/** + * A package that was removed from the manifest. + */ +export type RemovedPackage = { + /** + * The name of the removed package. + */ + name: DomainName; + /** + * The version of the removed package. + */ + version: SemanticVersion | PackageUrl; +}; + +/** + * Removes a dependency from a project manifest. This function will also take + * care of appying changes to scoped registries and testables. + * In comparison {@link removeDependency} will only remove the dependency. + * @param manifest The manifest to remove the dependency from. + * @param packageName The name of the dependency to remove. + * @returns A result where the Ok case is a tuple with the updated manifest and + * information about the removed package. + */ +export function tryRemoveProjectDependency( + manifest: UnityProjectManifest, + packageName: DomainName +): Result<[UnityProjectManifest, RemovedPackage], PackumentNotFoundError> { + const versionInManifest = manifest.dependencies[packageName]; + if (versionInManifest === undefined) + return Err(new PackumentNotFoundError(packageName)); + + manifest = removeDependency(manifest, packageName); + manifest = removeScopeFromAllScopedRegistries(manifest, packageName); + manifest = removeEmptyScopedRegistries(manifest); + manifest = removeTestable(manifest, packageName); + + // Remove scoped registries property if empty + if (manifest.scopedRegistries?.length === 0) + manifest = omitKey(manifest, "scopedRegistries"); + // Remove testables property if empty + if (manifest.testables?.length === 0) + manifest = omitKey(manifest, "testables"); + + return Ok([manifest, { name: packageName, version: versionInManifest }]); +} diff --git a/src/services/remove-packages.ts b/src/services/remove-packages.ts index a994d3a9..bf1403bb 100644 --- a/src/services/remove-packages.ts +++ b/src/services/remove-packages.ts @@ -1,14 +1,9 @@ -import { AsyncResult, Err, Ok, Result } from "ts-results-es"; +import { AsyncResult, Ok, Result } from "ts-results-es"; import { PackumentNotFoundError } from "../common-errors"; +import { tryRemoveProjectDependency } from "../domain/dependency-management"; import { DomainName } from "../domain/domain-name"; import { PackageUrl } from "../domain/package-url"; -import { - removeDependency, - removeEmptyScopedRegistries, - removeScopeFromAllScopedRegistries, - removeTestable, - UnityProjectManifest, -} from "../domain/project-manifest"; +import { UnityProjectManifest } from "../domain/project-manifest"; import { SemanticVersion } from "../domain/semantic-version"; import { LoadProjectManifest, @@ -17,7 +12,6 @@ import { SaveProjectManifest, } from "../io/project-manifest-io"; import { DebugLog } from "../logging"; -import { omitKey } from "../utils/object-utils"; import { resultifyAsyncOp } from "../utils/result-utils"; /** @@ -55,30 +49,6 @@ export function RemovePackagesFromManifest( loadProjectManifest: LoadProjectManifest, saveProjectManifest: SaveProjectManifest ): 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 = removeScopeFromAllScopedRegistries(manifest, packageName); - manifest = removeEmptyScopedRegistries(manifest); - manifest = removeTestable(manifest, packageName); - - // Remove scoped registries property if empty - if (manifest.scopedRegistries?.length === 0) - manifest = omitKey(manifest, "scopedRegistries"); - // Remove testables property if empty - if (manifest.testables?.length === 0) - manifest = omitKey(manifest, "testables"); - - return Ok([manifest, { name: packageName, version: versionInManifest }]); - }; - function tryRemoveAll( manifest: UnityProjectManifest, packageNames: ReadonlyArray @@ -91,7 +61,7 @@ export function RemovePackagesFromManifest( const currentPackageName = packageNames[0]!; const remainingPackageNames = packageNames.slice(1); - return tryRemoveSingle(manifest, currentPackageName).andThen( + return tryRemoveProjectDependency(manifest, currentPackageName).andThen( ([updatedManifest, removedPackage]) => tryRemoveAll(updatedManifest, remainingPackageNames).map( ([finalManifest, removedPackages]) => [ diff --git a/test/unit/domain/dependency-management.test.ts b/test/unit/domain/dependency-management.test.ts new file mode 100644 index 00000000..8a993f95 --- /dev/null +++ b/test/unit/domain/dependency-management.test.ts @@ -0,0 +1,168 @@ +import fc from "fast-check"; +import { PackumentNotFoundError } from "../../../src/common-errors"; +import { tryRemoveProjectDependency } from "../../../src/domain/dependency-management"; +import { DomainName } from "../../../src/domain/domain-name"; +import { + hasDependency, + mapScopedRegistry, +} from "../../../src/domain/project-manifest"; +import { RegistryUrl } from "../../../src/domain/registry-url"; +import { makeScopedRegistry } from "../../../src/domain/scoped-registry"; +import { recordKeys } from "../../../src/utils/record-utils"; +import { arbDomainName } from "./domain-name.arb"; +import { + arbManifest, + arbManifestWithDependencyCount, + arbNonEmptyManifest, +} from "./project-manifest.arb"; + +describe("dependency management", () => { + describe("remove single", () => { + it("should return error for package that is not in manifest", () => { + fc.assert( + fc.property(arbManifest, arbDomainName, (manifest, packageName) => { + // In the rare case where the manifest has the dependency we cancel + // the test. + if (hasDependency(manifest, packageName)) return; + + const error = tryRemoveProjectDependency( + manifest, + packageName + ).unwrapErr(); + + expect(error).toEqual(new PackumentNotFoundError(packageName)); + }) + ); + }); + + it("should remove dependency", () => { + fc.assert( + fc.property(arbNonEmptyManifest, (manifest) => { + const packageName = recordKeys(manifest.dependencies)[0]!; + + const [updated] = tryRemoveProjectDependency( + manifest, + packageName + ).unwrap(); + + const hasDependency = recordKeys(updated.dependencies).includes( + packageName + ); + expect(hasDependency).toBeFalsy(); + }) + ); + }); + + it("should return removed version", () => { + fc.assert( + fc.property(arbNonEmptyManifest, (manifest) => { + const packageName = recordKeys(manifest.dependencies)[0]!; + const versionInManifest = manifest.dependencies[packageName]!; + + const [, removedPackage] = tryRemoveProjectDependency( + manifest, + packageName + ).unwrap(); + + expect(removedPackage).toEqual({ + name: packageName, + version: versionInManifest, + }); + }) + ); + }); + + it("should remove from scoped registries", () => { + fc.assert( + fc.property(arbNonEmptyManifest, (manifest) => { + const packageName = recordKeys(manifest.dependencies)[0]!; + + const [updated] = tryRemoveProjectDependency( + manifest, + packageName + ).unwrap(); + + const anyScopedRegistryHasScope = + updated.scopedRegistries?.some((it) => + it.scopes.includes(packageName) + ) ?? false; + expect(anyScopedRegistryHasScope).toBeFalsy(); + }) + ); + }); + + it("should remove empty scoped registries", () => { + fc.assert( + fc.property(arbManifestWithDependencyCount(1), (manifest) => { + const originalScopedRegistryUrl = manifest.scopedRegistries![0]!.url; + // Add a second scoped registry so that at least one non-empty registry + // will remain in the manifest. Otherwise it would fully remove the + // scoped registries property. + const otherRegistry = RegistryUrl.parse("http://other.registry"); + manifest = mapScopedRegistry(manifest, otherRegistry, () => + makeScopedRegistry("Other registry", otherRegistry, [ + DomainName.parse("com.some.package"), + ]) + ); + const packageName = recordKeys(manifest.dependencies)[0]!; + + const [updated] = tryRemoveProjectDependency( + manifest, + packageName + ).unwrap(); + + const hasOriginalScopedRegistry = updated.scopedRegistries!.some( + (it) => it.url === originalScopedRegistryUrl + ); + expect(hasOriginalScopedRegistry).toBeFalsy(); + }) + ); + }); + + it("should remove from testables", () => { + fc.assert( + fc.property(arbNonEmptyManifest, (manifest) => { + const packageName = recordKeys(manifest.dependencies)[0]!; + + const [updated] = tryRemoveProjectDependency( + manifest, + packageName + ).unwrap(); + + const hasTestable = updated.testables?.includes(packageName) ?? false; + expect(hasTestable).toBeFalsy(); + }) + ); + }); + + it("should remove scoped registries property if empty", () => { + fc.assert( + fc.property(arbManifestWithDependencyCount(1), (manifest) => { + const packageName = recordKeys(manifest.dependencies)[0]!; + + const [updated] = tryRemoveProjectDependency( + manifest, + packageName + ).unwrap(); + + expect(updated.scopedRegistries).not.toBeDefined(); + }) + ); + }); + + it("should remove testables property if empty", () => { + fc.assert( + fc.property(arbManifestWithDependencyCount(1), (manifest) => { + const packageName = recordKeys(manifest.dependencies)[0]!; + + const [updated] = tryRemoveProjectDependency( + manifest, + packageName + ).unwrap(); + + expect(updated.testables).not.toBeDefined(); + }) + ); + }); + }); +}); diff --git a/test/unit/services/remove-packages.test.ts b/test/unit/services/remove-packages.test.ts index cb80bc5d..e7a3c579 100644 --- a/test/unit/services/remove-packages.test.ts +++ b/test/unit/services/remove-packages.test.ts @@ -1,14 +1,12 @@ import path from "path"; import { PackumentNotFoundError } from "../../../src/common-errors"; +import type { RemovedPackage } from "../../../src/domain/dependency-management"; import { DomainName } from "../../../src/domain/domain-name"; import { LoadProjectManifest, SaveProjectManifest, } from "../../../src/io/project-manifest-io"; -import { - RemovedPackage, - RemovePackagesFromManifest, -} from "../../../src/services/remove-packages"; +import { RemovePackagesFromManifest } from "../../../src/services/remove-packages"; import { buildProjectManifest } from "../domain/data-project-manifest"; import { mockFunctionOfType } from "./func.mock"; From d0a860da2988f8cce3d891d2c33b0c1f78a7a3be Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Sun, 1 Sep 2024 11:27:24 +0200 Subject: [PATCH 10/17] refactor: extract pure logic --- src/domain/dependency-management.ts | 32 ++++++++- src/services/remove-packages.ts | 29 +------- .../unit/domain/dependency-management.test.ts | 69 ++++++++++++++++++- 3 files changed, 102 insertions(+), 28 deletions(-) diff --git a/src/domain/dependency-management.ts b/src/domain/dependency-management.ts index 57aae55c..8821491d 100644 --- a/src/domain/dependency-management.ts +++ b/src/domain/dependency-management.ts @@ -11,7 +11,6 @@ import { removeTestable, } from "./project-manifest"; import type { SemanticVersion } from "./semantic-version"; - /** * A package that was removed from the manifest. */ @@ -57,3 +56,34 @@ export function tryRemoveProjectDependency( return Ok([manifest, { name: packageName, version: versionInManifest }]); } + +/** + * Atomically removes multiple dependencies from a project manifest using + * {@link tryRemoveProjectDependency}. + * @param manifest The manifest to remove the dependencies from. + * @param packageNames The names of the dependencies to remove. + * @returns A result where the Ok case is a tuple with the updated manifest and + * information about the removed packages. + */ +export function tryRemoveProjectDependencies( + manifest: UnityProjectManifest, + packageNames: ReadonlyArray +): Result< + [UnityProjectManifest, ReadonlyArray], + PackumentNotFoundError +> { + if (packageNames.length == 0) return Ok([manifest, []]); + + const currentPackageName = packageNames[0]!; + const remainingPackageNames = packageNames.slice(1); + + return tryRemoveProjectDependency(manifest, currentPackageName).andThen( + ([updatedManifest, removedPackage]) => + tryRemoveProjectDependencies(updatedManifest, remainingPackageNames).map( + ([finalManifest, removedPackages]) => [ + finalManifest, + [removedPackage, ...removedPackages], + ] + ) + ); +} diff --git a/src/services/remove-packages.ts b/src/services/remove-packages.ts index bf1403bb..96cfc41f 100644 --- a/src/services/remove-packages.ts +++ b/src/services/remove-packages.ts @@ -1,6 +1,6 @@ -import { AsyncResult, Ok, Result } from "ts-results-es"; +import { AsyncResult } from "ts-results-es"; import { PackumentNotFoundError } from "../common-errors"; -import { tryRemoveProjectDependency } from "../domain/dependency-management"; +import { tryRemoveProjectDependencies } from "../domain/dependency-management"; import { DomainName } from "../domain/domain-name"; import { PackageUrl } from "../domain/package-url"; import { UnityProjectManifest } from "../domain/project-manifest"; @@ -49,29 +49,6 @@ export function RemovePackagesFromManifest( loadProjectManifest: LoadProjectManifest, saveProjectManifest: SaveProjectManifest ): RemovePackages { - function tryRemoveAll( - manifest: UnityProjectManifest, - packageNames: ReadonlyArray - ): Result< - [UnityProjectManifest, ReadonlyArray], - PackumentNotFoundError - > { - if (packageNames.length == 0) return Ok([manifest, []]); - - const currentPackageName = packageNames[0]!; - const remainingPackageNames = packageNames.slice(1); - - return tryRemoveProjectDependency(manifest, currentPackageName).andThen( - ([updatedManifest, removedPackage]) => - tryRemoveAll(updatedManifest, remainingPackageNames).map( - ([finalManifest, removedPackages]) => [ - finalManifest, - [removedPackage, ...removedPackages], - ] - ) - ); - } - return (projectPath, packageNames) => { // load manifest const initialManifest = resultifyAsyncOp< @@ -81,7 +58,7 @@ export function RemovePackagesFromManifest( // remove const removeResult = initialManifest.andThen((it) => - tryRemoveAll(it, packageNames) + tryRemoveProjectDependencies(it, packageNames) ); return removeResult.map(async ([updatedManifest, removedPackages]) => { diff --git a/test/unit/domain/dependency-management.test.ts b/test/unit/domain/dependency-management.test.ts index 8a993f95..36e9a6f7 100644 --- a/test/unit/domain/dependency-management.test.ts +++ b/test/unit/domain/dependency-management.test.ts @@ -1,6 +1,9 @@ import fc from "fast-check"; import { PackumentNotFoundError } from "../../../src/common-errors"; -import { tryRemoveProjectDependency } from "../../../src/domain/dependency-management"; +import { + tryRemoveProjectDependencies, + tryRemoveProjectDependency, +} from "../../../src/domain/dependency-management"; import { DomainName } from "../../../src/domain/domain-name"; import { hasDependency, @@ -165,4 +168,68 @@ describe("dependency management", () => { ); }); }); + + describe("remove multiple", () => { + it("should have no effect for empty package list", () => { + fc.assert( + fc.property(arbManifest, (manifest) => { + const [updated] = tryRemoveProjectDependencies(manifest, []).unwrap(); + + expect(updated).toEqual(manifest); + }) + ); + }); + + it("should remove packages", () => { + fc.assert( + fc.property(arbManifestWithDependencyCount(10), (manifest) => { + const packagesToRemove = recordKeys(manifest.dependencies).slice( + 0, + 5 + ); + + const [updated, removed] = tryRemoveProjectDependencies( + manifest, + packagesToRemove + ).unwrap(); + + // Here we only do some surface level tests to check that the + // packages were removed correctly. More detailed tests are in + // the section for the "remove single" function because the + // "remove multiple" version uses that internally. + + packagesToRemove.forEach((it) => { + const hasDependency = recordKeys(updated.dependencies).includes(it); + expect(hasDependency).toEqual(false); + }); + expect(removed.map((it) => it.name)).toEqual(packagesToRemove); + }) + ); + }); + + it("should should be atomic", () => { + fc.assert( + fc.property( + arbManifestWithDependencyCount(10), + arbDomainName, + (manifest, missingPackage) => { + // A mix of packages that are in the package and one + // which is not. + const packagesToRemove = [ + ...recordKeys(manifest.dependencies).slice(0, 2), + missingPackage, + ...recordKeys(manifest.dependencies).slice(3, 5), + ]; + + const error = tryRemoveProjectDependencies( + manifest, + packagesToRemove + ).unwrapErr(); + + expect(error).toEqual(new PackumentNotFoundError(missingPackage)); + } + ) + ); + }); + }); }); From 20a72fb51cd10a15946198bb8270f93b765c4579 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Sun, 1 Sep 2024 11:28:00 +0200 Subject: [PATCH 11/17] test: remove duplicate tests --- test/unit/services/remove-packages.test.ts | 106 --------------------- 1 file changed, 106 deletions(-) delete mode 100644 test/unit/services/remove-packages.test.ts diff --git a/test/unit/services/remove-packages.test.ts b/test/unit/services/remove-packages.test.ts deleted file mode 100644 index e7a3c579..00000000 --- a/test/unit/services/remove-packages.test.ts +++ /dev/null @@ -1,106 +0,0 @@ -import path from "path"; -import { PackumentNotFoundError } from "../../../src/common-errors"; -import type { RemovedPackage } from "../../../src/domain/dependency-management"; -import { DomainName } from "../../../src/domain/domain-name"; -import { - LoadProjectManifest, - SaveProjectManifest, -} from "../../../src/io/project-manifest-io"; -import { RemovePackagesFromManifest } from "../../../src/services/remove-packages"; -import { buildProjectManifest } from "../domain/data-project-manifest"; -import { mockFunctionOfType } from "./func.mock"; - -describe("remove packages from manifest", () => { - const someProjectPath = path.resolve("/home/projects/MyUnityProject"); - const somePackage = DomainName.parse("com.some.package"); - const otherPackage = DomainName.parse("com.other.package"); - - const defaultManifest = buildProjectManifest((manifest) => - manifest.addDependency(somePackage, "1.0.0", true, true) - ); - - function makeDependencies() { - const loadProjectManifest = mockFunctionOfType(); - loadProjectManifest.mockResolvedValue(defaultManifest); - - const writeProjectManifest = mockFunctionOfType(); - writeProjectManifest.mockResolvedValue(undefined); - - const removePackagesFromManifestu = RemovePackagesFromManifest( - loadProjectManifest, - writeProjectManifest - ); - return { - removePackagesFromManifestu, - loadProjectManifest, - writeProjectManifest, - } as const; - } - - it("should fail if package is not in manifest", async () => { - const { removePackagesFromManifestu } = makeDependencies(); - - const result = await removePackagesFromManifestu(someProjectPath, [ - otherPackage, - ]).promise; - - expect(result).toBeError((actual) => - expect(actual).toBeInstanceOf(PackumentNotFoundError) - ); - }); - - it("should return removed version", async () => { - const { removePackagesFromManifestu } = makeDependencies(); - - const result = await removePackagesFromManifestu(someProjectPath, [ - somePackage, - ]).promise; - - expect(result).toBeOk((removedPackage: RemovedPackage) => - expect(removedPackage).toEqual([{ name: somePackage, version: "1.0.0" }]) - ); - }); - - it("should be atomic for multiple packages", async () => { - const { removePackagesFromManifestu, writeProjectManifest } = - makeDependencies(); - - // One of these packages can not be removed, so none should be removed. - await removePackagesFromManifestu(someProjectPath, [ - somePackage, - otherPackage, - ]).promise; - - expect(writeProjectManifest).not.toHaveBeenCalled(); - }); - - it("should remove package from manifest", async () => { - const { removePackagesFromManifestu, writeProjectManifest } = - makeDependencies(); - - await removePackagesFromManifestu(someProjectPath, [somePackage]).promise; - - expect(writeProjectManifest).toHaveBeenCalledWith( - expect.any(String), - expect.not.objectContaining({ - dependencies: { - [somePackage]: expect.anything(), - }, - }) - ); - }); - - it("should remove scope from manifest", async () => { - const { removePackagesFromManifestu, writeProjectManifest } = - makeDependencies(); - - await removePackagesFromManifestu(someProjectPath, [somePackage]).promise; - - expect(writeProjectManifest).toHaveBeenCalledWith( - expect.any(String), - expect.not.objectContaining({ - scopes: [somePackage], - }) - ); - }); -}); From 4dbe7edf72c490113a0e342d30cf0b4fd385cc15 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Sun, 1 Sep 2024 11:33:59 +0200 Subject: [PATCH 12/17] deps(ci): bump actions (#389) Bump version of actions to get rid of deprecation warnings --- .github/workflows/ci.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 18ff214e..46a212f9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -7,7 +7,7 @@ jobs: runs-on: ubuntu-latest name: Unit Tests steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - name: Setup Node.js uses: actions/setup-node@v3 with: @@ -30,9 +30,9 @@ jobs: - "22.x" # Latest name: E2E Tests steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - name: setup node.js - uses: actions/setup-node@v3 + uses: actions/setup-node@v4 with: node-version: ${{ matrix.node }} - name: install deps @@ -49,7 +49,7 @@ jobs: - e2e-test if: github.ref == 'refs/heads/master' steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - name: npm install run: npm install - name: Build From e46e1140668350576aecafed6921a59d9cb832d0 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Sun, 1 Sep 2024 12:33:42 +0200 Subject: [PATCH 13/17] refactor: extract pure --- src/services/login.ts | 4 +- src/services/put-registry-auth.ts | 83 ++++++---- test/unit/services/put-registry-auth.test.ts | 166 +++++++------------ 3 files changed, 115 insertions(+), 138 deletions(-) diff --git a/src/services/login.ts b/src/services/login.ts index 5757aedf..8067c3b1 100644 --- a/src/services/login.ts +++ b/src/services/login.ts @@ -3,7 +3,7 @@ import { RegistryUrl } from "../domain/registry-url"; import { DebugLog } from "../logging"; import { getAuthToken, GetAuthToken } from "./get-auth-token"; import { putNpmAuthToken, StoreNpmAuthToken } from "./put-npm-auth-token"; -import { putRegistryAuth, PutRegistryAuth } from "./put-registry-auth"; +import { putRegistryAuthIntoUserUpmConfig, PutRegistryAuth } from "./put-registry-auth"; /** * Function for logging in a user to a package registry. Supports both basic and @@ -72,7 +72,7 @@ export const login = ( debugLog: DebugLog ) => UpmConfigLogin( - putRegistryAuth, + putRegistryAuthIntoUserUpmConfig, getAuthToken(registryClient, debugLog), putNpmAuthToken(homePath), debugLog diff --git a/src/services/put-registry-auth.ts b/src/services/put-registry-auth.ts index 2b963bf4..743519e8 100644 --- a/src/services/put-registry-auth.ts +++ b/src/services/put-registry-auth.ts @@ -24,6 +24,52 @@ export type PutRegistryAuth = ( auth: NpmAuth ) => Promise; +function mergeEntries(oldEntry: UpmAuth | null, newEntry: NpmAuth): UpmAuth { + const alwaysAuth = newEntry.alwaysAuth ?? oldEntry?.alwaysAuth; + + if ("token" in newEntry) { + return removeExplicitUndefined({ + token: newEntry.token, + email: oldEntry?.email, + alwaysAuth, + }); + } + + return removeExplicitUndefined({ + _auth: encodeBase64(`${newEntry.username}:${newEntry.password}`), + email: newEntry.email, + alwaysAuth, + }); +} + +/** + * Updates a {@link UpmConfigContent} object to contain auth data for a + * specific registry. + * @param currentContent The current upm config content. May be null if there + * is no config currently. + * @param registry The registry for which to put the auth data. + * @param auth The auth data. + * @returns An updated upm config content with the auth data. + */ +export function putRegistryAuthIntoUpmConfig( + currentContent: UpmConfigContent | null, + registry: RegistryUrl, + auth: NpmAuth +): UpmConfigContent { + const oldEntries = currentContent?.npmAuth ?? {}; + // Search the entry both with and without trailing slash + const oldEntry = oldEntries[registry] ?? oldEntries[registry + "/"] ?? null; + const newContent: UpmConfigContent = removeExplicitUndefined({ + npmAuth: { + ...oldEntries, + // Remove entry with trailing slash + [registry + "/"]: undefined, + [registry]: mergeEntries(oldEntry, auth), + }, + }); + return newContent; +} + /** * Makes a {@link PutRegistryAuth} function which puts registry authentication * info into the users upm config. @@ -32,38 +78,15 @@ export function PutRegistryAuthIntoUpmConfig( loadUpmConfig: LoadUpmConfig, saveUpmConfig: SaveUpmConfig ): PutRegistryAuth { - function mergeEntries(oldEntry: UpmAuth | null, newEntry: NpmAuth): UpmAuth { - const alwaysAuth = newEntry.alwaysAuth ?? oldEntry?.alwaysAuth; - - if ("token" in newEntry) { - return removeExplicitUndefined({ - token: newEntry.token, - email: oldEntry?.email, - alwaysAuth, - }); - } - - return removeExplicitUndefined({ - _auth: encodeBase64(`${newEntry.username}:${newEntry.password}`), - email: newEntry.email, - alwaysAuth, - }); - } - return async (configPath, registry, auth) => { const currentContent = await loadUpmConfig(configPath); - const oldEntries = currentContent?.npmAuth ?? {}; - // Search the entry both with and without trailing slash - const oldEntry = oldEntries[registry] ?? oldEntries[registry + "/"] ?? null; - const newContent: UpmConfigContent = removeExplicitUndefined({ - npmAuth: { - ...oldEntries, - // Remove entry with trailing slash - [registry + "/"]: undefined, - [registry]: mergeEntries(oldEntry, auth), - }, - }); + const newContent: UpmConfigContent = putRegistryAuthIntoUpmConfig( + currentContent, + registry, + auth + ); + await saveUpmConfig(newContent, configPath); }; } @@ -71,7 +94,7 @@ export function PutRegistryAuthIntoUpmConfig( /** * Default {@link PutRegistryAuth} function. Uses {@link PutRegistryAuthIntoUpmConfig}. */ -export const putRegistryAuth = PutRegistryAuthIntoUpmConfig( +export const putRegistryAuthIntoUserUpmConfig = PutRegistryAuthIntoUpmConfig( loadUpmConfig, saveUpmConfig ); diff --git a/test/unit/services/put-registry-auth.test.ts b/test/unit/services/put-registry-auth.test.ts index ca1de26d..3027c38b 100644 --- a/test/unit/services/put-registry-auth.test.ts +++ b/test/unit/services/put-registry-auth.test.ts @@ -1,194 +1,148 @@ -import { mockFunctionOfType } from "./func.mock"; -import { LoadUpmConfig, SaveUpmConfig } from "../../../src/io/upm-config-io"; -import { exampleRegistryUrl } from "../domain/data-registry"; import { Base64 } from "../../../src/domain/base64"; -import { PutRegistryAuthIntoUpmConfig } from "../../../src/services/put-registry-auth"; +import { type UpmConfigContent } from "../../../src/io/upm-config-io"; +import { putRegistryAuthIntoUpmConfig } from "../../../src/services/put-registry-auth"; +import { exampleRegistryUrl } from "../domain/data-registry"; describe("put registry auth into upm config", () => { - const someConfigPath = "/home/user/.upmconfig.toml"; const someEmail = "user@mail.com"; const someToken = "isehusehgusheguszg8gshg"; const otherToken = "8zseg974wge4g94whfheghf"; - function makeDependencies() { - const loadUpmConfig = mockFunctionOfType(); - loadUpmConfig.mockResolvedValue({}); - - const saveUpmConfig = mockFunctionOfType(); - saveUpmConfig.mockResolvedValue(); - - const putRegistryAuth = PutRegistryAuthIntoUpmConfig( - loadUpmConfig, - saveUpmConfig - ); - return { putRegistryAuth, loadUpmConfig, saveUpmConfig } as const; - } - - it("should add entry if it does not exist", async () => { - const { putRegistryAuth, saveUpmConfig } = makeDependencies(); - - await putRegistryAuth(someConfigPath, exampleRegistryUrl, { + it("should add entry if it does not exist", () => { + const actual = putRegistryAuthIntoUpmConfig(null, exampleRegistryUrl, { token: someToken, }); - expect(saveUpmConfig).toHaveBeenCalledWith( - { - npmAuth: { - [exampleRegistryUrl]: { - token: someToken, - }, + expect(actual).toEqual({ + npmAuth: { + [exampleRegistryUrl]: { + token: someToken, }, }, - someConfigPath - ); + }); }); - it("should replace entry of same type", async () => { - const { putRegistryAuth, loadUpmConfig, saveUpmConfig } = - makeDependencies(); - loadUpmConfig.mockResolvedValue({ + it("should replace entry of same type", () => { + const initial: UpmConfigContent = { npmAuth: { [exampleRegistryUrl]: { _auth: "dXNlcjpwYXNz" as Base64, // user:pass email: "other@email.com", }, }, - }); + }; - await putRegistryAuth(someConfigPath, exampleRegistryUrl, { + const actual = putRegistryAuthIntoUpmConfig(initial, exampleRegistryUrl, { username: "user", password: "pass", email: someEmail, }); - expect(saveUpmConfig).toHaveBeenCalledWith( - { - npmAuth: { - [exampleRegistryUrl]: { - _auth: "dXNlcjpwYXNz" as Base64, // user:pass - email: someEmail, - }, + expect(actual).toEqual({ + npmAuth: { + [exampleRegistryUrl]: { + _auth: "dXNlcjpwYXNz" as Base64, // user:pass + email: someEmail, }, }, - someConfigPath - ); + }); }); - it("should replace entry of different type", async () => { - const { putRegistryAuth, loadUpmConfig, saveUpmConfig } = - makeDependencies(); - loadUpmConfig.mockResolvedValue({ + it("should replace entry of different type", () => { + const initial: UpmConfigContent = { npmAuth: { [exampleRegistryUrl]: { token: someToken, }, }, - }); + }; - await putRegistryAuth(someConfigPath, exampleRegistryUrl, { + const actual = putRegistryAuthIntoUpmConfig(initial, exampleRegistryUrl, { username: "user", password: "pass", email: someEmail, }); - expect(saveUpmConfig).toHaveBeenCalledWith( - { - npmAuth: { - [exampleRegistryUrl]: { - _auth: "dXNlcjpwYXNz" as Base64, // user:pass - email: someEmail, - }, + expect(actual).toEqual({ + npmAuth: { + [exampleRegistryUrl]: { + _auth: "dXNlcjpwYXNz" as Base64, // user:pass + email: someEmail, }, }, - someConfigPath - ); + }); }); - it("should replace entry for url that has trailing slash", async () => { - const { putRegistryAuth, loadUpmConfig, saveUpmConfig } = - makeDependencies(); - loadUpmConfig.mockResolvedValue({ + it("should replace entry for url that has trailing slash", () => { + const initial: UpmConfigContent = { // This entry has an url with a trailing slash, but it should // still be replaced. [exampleRegistryUrl + "/"]: { token: someToken, }, - }); + }; - await putRegistryAuth(someConfigPath, exampleRegistryUrl, { + const actual = putRegistryAuthIntoUpmConfig(initial, exampleRegistryUrl, { username: "user", password: "pass", email: someEmail, }); - expect(saveUpmConfig).toHaveBeenCalledWith( - { - npmAuth: { - [exampleRegistryUrl]: { - _auth: "dXNlcjpwYXNz" as Base64, // user:pass - email: someEmail, - }, + expect(actual).toEqual({ + npmAuth: { + [exampleRegistryUrl]: { + _auth: "dXNlcjpwYXNz" as Base64, // user:pass + email: someEmail, }, }, - someConfigPath - ); + }); }); - it("should keep email of token entry", async () => { - const { putRegistryAuth, loadUpmConfig, saveUpmConfig } = - makeDependencies(); - loadUpmConfig.mockResolvedValue({ + it("should keep email of token entry", () => { + const initial: UpmConfigContent = { npmAuth: { [exampleRegistryUrl]: { token: someToken, email: someEmail, }, }, - }); + }; - await putRegistryAuth(someConfigPath, exampleRegistryUrl, { + const actual = putRegistryAuthIntoUpmConfig(initial, exampleRegistryUrl, { token: otherToken, }); - expect(saveUpmConfig).toHaveBeenCalledWith( - { - npmAuth: { - [exampleRegistryUrl]: { - token: otherToken, - email: someEmail, - }, + expect(actual).toEqual({ + npmAuth: { + [exampleRegistryUrl]: { + token: otherToken, + email: someEmail, }, }, - someConfigPath - ); + }); }); - it("should keep always auth if no replacement was provided", async () => { - const { putRegistryAuth, loadUpmConfig, saveUpmConfig } = - makeDependencies(); - loadUpmConfig.mockResolvedValue({ + it("should keep always auth if no replacement was provided", () => { + const initial: UpmConfigContent = { npmAuth: { [exampleRegistryUrl]: { token: someToken, alwaysAuth: true, }, }, - }); + }; - await putRegistryAuth(someConfigPath, exampleRegistryUrl, { + const actual = putRegistryAuthIntoUpmConfig(initial, exampleRegistryUrl, { token: otherToken, }); - expect(saveUpmConfig).toHaveBeenCalledWith( - { - npmAuth: { - [exampleRegistryUrl]: { - token: otherToken, - alwaysAuth: true, - }, + expect(actual).toEqual({ + npmAuth: { + [exampleRegistryUrl]: { + token: otherToken, + alwaysAuth: true, }, }, - someConfigPath - ); + }); }); }); From 5582f8ca925fdd98fbb1ee1d9ceb2bd9c6c3c5ff Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Sun, 1 Sep 2024 12:46:02 +0200 Subject: [PATCH 14/17] refactor: extract pure --- src/services/get-registry-auth.ts | 63 +++--- test/unit/services/get-registry-auth.test.ts | 193 ++++++++++--------- 2 files changed, 140 insertions(+), 116 deletions(-) diff --git a/src/services/get-registry-auth.ts b/src/services/get-registry-auth.ts index 252c9af6..f29b1984 100644 --- a/src/services/get-registry-auth.ts +++ b/src/services/get-registry-auth.ts @@ -28,6 +28,44 @@ export type GetRegistryAuth = ( url: RegistryUrl ) => Promise; +/** + * Checks whether a registry requires authentication. This just checks whether + * the url is for the Unity or OpenUPM registry. + * @param url The url to check. + * @returns Whether the url is known to not require authentication. + */ +export function isNonAuthUrl(url: RegistryUrl): boolean { + // We know there is no auth required for these registries + return url === openupmRegistryUrl || url === unityRegistryUrl; +} + +/** + * Converts a {@link UpmAuth} object to an {@link NpmAuth} object. + * @param auth The input auth object to convert. + * @returns The converted auth object. + * @throws {Error} If auth contained bad base64 string. + */ +export function importNpmAuth(auth: UpmAuth): NpmAuth { + // Basic auth + if ("_auth" in auth) { + const decoded = decodeBase64(auth._auth); + const [username, password] = trySplitAtFirstOccurrenceOf(decoded, ":"); + if (password === null) + throw new Error("Auth Base64 string was not in the user:pass format."); + return removeExplicitUndefined({ + username, + password, + email: auth.email, + alwaysAuth: auth.alwaysAuth, + }); + } + + return removeExplicitUndefined({ + token: auth.token, + alwaysAuth: auth.alwaysAuth, + }); +} + /** * Makes a {@link GetRegistryAuth} function which gets it's information from * the users upm config. @@ -37,33 +75,10 @@ export function LoadRegistryAuthFromUpmConfig( loadUpmConfig: LoadUpmConfig, debugLog: DebugLog ): GetRegistryAuth { - function importNpmAuth(input: UpmAuth): NpmAuth { - // Basic auth - if ("_auth" in input) { - const decoded = decodeBase64(input._auth); - const [username, password] = trySplitAtFirstOccurrenceOf(decoded, ":"); - if (password === null) - throw new Error("Auth Base64 string was not in the user:pass format."); - return removeExplicitUndefined({ - username, - password, - email: input.email, - alwaysAuth: input.alwaysAuth, - }); - } - - return removeExplicitUndefined({ - token: input.token, - alwaysAuth: input.alwaysAuth, - }); - } - let cachedConfig: UpmConfigContent | null = null; return async (systemUser, url) => { - // We know there is no auth required for these registries - if (url === openupmRegistryUrl || url === unityRegistryUrl) - return { url, auth: null }; + if (isNonAuthUrl(url)) return { url, auth: null }; // Only load config if we have dont have it in the cache if (cachedConfig === null) { diff --git a/test/unit/services/get-registry-auth.test.ts b/test/unit/services/get-registry-auth.test.ts index ffd36b73..dda6c690 100644 --- a/test/unit/services/get-registry-auth.test.ts +++ b/test/unit/services/get-registry-auth.test.ts @@ -5,7 +5,11 @@ import { } from "../../../src/domain/registry-url"; import { GetUpmConfigPath, LoadUpmConfig } from "../../../src/io/upm-config-io"; import { noopLogger } from "../../../src/logging"; -import { LoadRegistryAuthFromUpmConfig } from "../../../src/services/get-registry-auth"; +import { + importNpmAuth, + isNonAuthUrl, + LoadRegistryAuthFromUpmConfig, +} from "../../../src/services/get-registry-auth"; import { exampleRegistryUrl } from "../domain/data-registry"; import { mockFunctionOfType } from "./func.mock"; @@ -13,128 +17,133 @@ describe("get registry auth from upm config", () => { const someEmail = "user@mail.com"; const someToken = "isehusehgusheguszg8gshg"; - function makeDependencies() { - const getUpmConfigPath = mockFunctionOfType(); - getUpmConfigPath.mockReturnValue("/home/user/.upmconfig.toml"); - - const loadUpmConfig = mockFunctionOfType(); - loadUpmConfig.mockResolvedValue({}); - - const getRegistryAuth = LoadRegistryAuthFromUpmConfig( - getUpmConfigPath, - loadUpmConfig, - noopLogger - ); - return { getRegistryAuth, loadUpmConfig } as const; - } - - it("should have no auth if no .upmconfig.toml file", async () => { - const { getRegistryAuth, loadUpmConfig } = makeDependencies(); - loadUpmConfig.mockResolvedValue(null); + describe("registry needs no auth", () => { + it("should be true for Unity registry", () => { + const actual = isNonAuthUrl(unityRegistryUrl); + expect(actual).toBeTruthy(); + }); - const registry = await getRegistryAuth(false, exampleRegistryUrl); + it("should be true for OpenUPM registry", () => { + const actual = isNonAuthUrl(openupmRegistryUrl); + expect(actual).toBeTruthy(); + }); - expect(registry.auth).toBeNull(); + it("should be false for other registries", () => { + const actual = isNonAuthUrl(exampleRegistryUrl); + expect(actual).toBeFalsy(); + }); }); - it("should have no auth if there is no entry for registry", async () => { - const { getRegistryAuth, loadUpmConfig } = makeDependencies(); - loadUpmConfig.mockResolvedValue({}); - - const registry = await getRegistryAuth(false, exampleRegistryUrl); + describe("auth import", () => { + it("should get valid basic auth", () => { + const actual = importNpmAuth({ + _auth: "dXNlcjpwYXNz" as Base64, // user:pass + email: someEmail, + alwaysAuth: true, + }); + + expect(actual).toEqual({ + username: "user", + password: "pass", + email: someEmail, + alwaysAuth: true, + }); + }); - expect(registry.auth).toBeNull(); - }); + it("should get valid token auth", () => { + const auth = importNpmAuth({ + token: someToken, + alwaysAuth: true, + }); - it("should get valid basic auth", async () => { - const { getRegistryAuth, loadUpmConfig } = makeDependencies(); - loadUpmConfig.mockResolvedValue({ - npmAuth: { - [exampleRegistryUrl]: { - _auth: "dXNlcjpwYXNz" as Base64, // user:pass - email: someEmail, - alwaysAuth: true, - }, - }, + expect(auth).toEqual({ + token: someToken, + alwaysAuth: true, + }); }); - const registry = await getRegistryAuth(false, exampleRegistryUrl); + it("should ignore email when getting token auth", () => { + const auth = importNpmAuth({ + token: someToken, + email: someEmail, + }); - expect(registry.auth).toEqual({ - username: "user", - password: "pass", - email: someEmail, - alwaysAuth: true, + expect(auth).toEqual({ + token: someToken, + }); }); }); - it("should get valid token auth", async () => { - const { getRegistryAuth, loadUpmConfig } = makeDependencies(); - loadUpmConfig.mockResolvedValue({ - npmAuth: { [exampleRegistryUrl]: { token: someToken, alwaysAuth: true } }, - }); + describe("service", () => { + function makeDependencies() { + const getUpmConfigPath = mockFunctionOfType(); + getUpmConfigPath.mockReturnValue("/home/user/.upmconfig.toml"); - const registry = await getRegistryAuth(false, exampleRegistryUrl); + const loadUpmConfig = mockFunctionOfType(); + loadUpmConfig.mockResolvedValue({}); - expect(registry.auth).toEqual({ - token: someToken, - alwaysAuth: true, - }); - }); + const getRegistryAuth = LoadRegistryAuthFromUpmConfig( + getUpmConfigPath, + loadUpmConfig, + noopLogger + ); + return { getRegistryAuth, loadUpmConfig } as const; + } - it("should ignore email when getting token auth", async () => { - const { getRegistryAuth, loadUpmConfig } = makeDependencies(); - loadUpmConfig.mockResolvedValue({ - npmAuth: { - [exampleRegistryUrl]: { - token: someToken, - email: someEmail, - }, - }, - }); + it("should have no auth if no .upmconfig.toml file", async () => { + const { getRegistryAuth, loadUpmConfig } = makeDependencies(); + loadUpmConfig.mockResolvedValue(null); - const registry = await getRegistryAuth(false, exampleRegistryUrl); + const registry = await getRegistryAuth(false, exampleRegistryUrl); - expect(registry.auth).toEqual({ - token: someToken, + expect(registry.auth).toBeNull(); }); - }); - it("should get auth for url with trailing slash", async () => { - const { getRegistryAuth, loadUpmConfig } = makeDependencies(); - loadUpmConfig.mockResolvedValue({ - npmAuth: { [exampleRegistryUrl + "/"]: { token: someToken } }, + it("should have no auth if there is no entry for registry", async () => { + const { getRegistryAuth, loadUpmConfig } = makeDependencies(); + loadUpmConfig.mockResolvedValue({}); + + const registry = await getRegistryAuth(false, exampleRegistryUrl); + + expect(registry.auth).toBeNull(); }); - const registry = await getRegistryAuth(false, exampleRegistryUrl); + it("should get auth for url with trailing slash", async () => { + const { getRegistryAuth, loadUpmConfig } = makeDependencies(); + loadUpmConfig.mockResolvedValue({ + npmAuth: { [exampleRegistryUrl + "/"]: { token: someToken } }, + }); + + const registry = await getRegistryAuth(false, exampleRegistryUrl); - expect(registry.auth).toEqual({ - token: someToken, + expect(registry.auth).toEqual({ + token: someToken, + }); }); - }); - it("should not load upmconfig for openupm registry url", async () => { - const { getRegistryAuth, loadUpmConfig } = makeDependencies(); + it("should not load upmconfig for openupm registry url", async () => { + const { getRegistryAuth, loadUpmConfig } = makeDependencies(); - await getRegistryAuth(false, openupmRegistryUrl); + await getRegistryAuth(false, openupmRegistryUrl); - expect(loadUpmConfig).not.toHaveBeenCalled(); - }); + expect(loadUpmConfig).not.toHaveBeenCalled(); + }); - it("should not load upmconfig for unity registry url", async () => { - const { getRegistryAuth, loadUpmConfig } = makeDependencies(); + it("should not load upmconfig for unity registry url", async () => { + const { getRegistryAuth, loadUpmConfig } = makeDependencies(); - await getRegistryAuth(false, unityRegistryUrl); + await getRegistryAuth(false, unityRegistryUrl); - expect(loadUpmConfig).not.toHaveBeenCalled(); - }); + expect(loadUpmConfig).not.toHaveBeenCalled(); + }); - it("should cache .upmconfig.toml content", async () => { - const { getRegistryAuth, loadUpmConfig } = makeDependencies(); + it("should cache .upmconfig.toml content", async () => { + const { getRegistryAuth, loadUpmConfig } = makeDependencies(); - await getRegistryAuth(false, exampleRegistryUrl); - await getRegistryAuth(false, exampleRegistryUrl); + await getRegistryAuth(false, exampleRegistryUrl); + await getRegistryAuth(false, exampleRegistryUrl); - expect(loadUpmConfig).toHaveBeenCalledTimes(1); + expect(loadUpmConfig).toHaveBeenCalledTimes(1); + }); }); }); From b6a0dca5bb6096cc87cf363cffc5466cdfb04895 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Sun, 1 Sep 2024 12:52:25 +0200 Subject: [PATCH 15/17] refactor: move files move io functions into io folder --- src/{services => io}/get-auth-token.ts | 2 +- src/services/login.ts | 2 +- test/unit/{services => io}/get-auth-token.test.ts | 2 +- test/unit/io/packument-io.test.ts | 2 +- test/unit/{services => io}/registry-client.mock.ts | 0 test/unit/services/login.test.ts | 2 +- 6 files changed, 5 insertions(+), 5 deletions(-) rename src/{services => io}/get-auth-token.ts (96%) rename test/unit/{services => io}/get-auth-token.test.ts (96%) rename test/unit/{services => io}/registry-client.mock.ts (100%) diff --git a/src/services/get-auth-token.ts b/src/io/get-auth-token.ts similarity index 96% rename from src/services/get-auth-token.ts rename to src/io/get-auth-token.ts index 05d315b9..8c9ef671 100644 --- a/src/services/get-auth-token.ts +++ b/src/io/get-auth-token.ts @@ -1,6 +1,6 @@ import RegClient from "another-npm-registry-client"; import { RegistryUrl } from "../domain/registry-url"; -import { RegistryAuthenticationError } from "../io/common-errors"; +import { RegistryAuthenticationError } from "./common-errors"; import { DebugLog } from "../logging"; /** diff --git a/src/services/login.ts b/src/services/login.ts index 8067c3b1..2645c044 100644 --- a/src/services/login.ts +++ b/src/services/login.ts @@ -1,7 +1,7 @@ import RegClient, { NpmAuth } from "another-npm-registry-client"; import { RegistryUrl } from "../domain/registry-url"; import { DebugLog } from "../logging"; -import { getAuthToken, GetAuthToken } from "./get-auth-token"; +import { getAuthToken, GetAuthToken } from "../io/get-auth-token"; import { putNpmAuthToken, StoreNpmAuthToken } from "./put-npm-auth-token"; import { putRegistryAuthIntoUserUpmConfig, PutRegistryAuth } from "./put-registry-auth"; diff --git a/test/unit/services/get-auth-token.test.ts b/test/unit/io/get-auth-token.test.ts similarity index 96% rename from test/unit/services/get-auth-token.test.ts rename to test/unit/io/get-auth-token.test.ts index 54f8895e..c8fc2360 100644 --- a/test/unit/services/get-auth-token.test.ts +++ b/test/unit/io/get-auth-token.test.ts @@ -4,7 +4,7 @@ import { exampleRegistryUrl } from "../domain/data-registry"; import { mockRegClientAddUserResult } from "./registry-client.mock"; import { RegistryAuthenticationError } from "../../../src/io/common-errors"; import { noopLogger } from "../../../src/logging"; -import { AuthenticateWithNpmRegistry } from "../../../src/services/get-auth-token"; +import { AuthenticateWithNpmRegistry } from "../../../src/io/get-auth-token"; describe("authenticate user with npm registry", () => { function makeDependencies() { diff --git a/test/unit/io/packument-io.test.ts b/test/unit/io/packument-io.test.ts index e6656caa..789f71b2 100644 --- a/test/unit/io/packument-io.test.ts +++ b/test/unit/io/packument-io.test.ts @@ -5,7 +5,7 @@ import { getRegistryPackumentUsing } from "../../../src/io/packument-io"; import { noopLogger } from "../../../src/logging"; import { buildPackument } from "../domain/data-packument"; import { exampleRegistryUrl } from "../domain/data-registry"; -import { mockRegClientGetResult } from "../services/registry-client.mock"; +import { mockRegClientGetResult } from "./registry-client.mock"; describe("packument io", () => { describe("fetch", () => { diff --git a/test/unit/services/registry-client.mock.ts b/test/unit/io/registry-client.mock.ts similarity index 100% rename from test/unit/services/registry-client.mock.ts rename to test/unit/io/registry-client.mock.ts diff --git a/test/unit/services/login.test.ts b/test/unit/services/login.test.ts index c166eb4a..1ef713c3 100644 --- a/test/unit/services/login.test.ts +++ b/test/unit/services/login.test.ts @@ -1,5 +1,5 @@ import { noopLogger } from "../../../src/logging"; -import { GetAuthToken } from "../../../src/services/get-auth-token"; +import { GetAuthToken } from "../../../src/io/get-auth-token"; import { UpmConfigLogin } from "../../../src/services/login"; import { StoreNpmAuthToken } from "../../../src/services/put-npm-auth-token"; import { PutRegistryAuth } from "../../../src/services/put-registry-auth"; From 13b557750fc54a56aad21337055f72a0e275a82f Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Sat, 7 Sep 2024 11:00:52 +0200 Subject: [PATCH 16/17] fix: partially resolved dependencies (#393) Fix dependency resolving logic quitting early when encountering a built-in or failed dependency. This fixes #393 --- src/services/dependency-resolving.ts | 4 ++-- test/e2e/add.test.ts | 25 +++++++++++++++++++++---- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/services/dependency-resolving.ts b/src/services/dependency-resolving.ts index be8e20f9..71bab90d 100644 --- a/src/services/dependency-resolving.ts +++ b/src/services/dependency-resolving.ts @@ -61,7 +61,7 @@ export function ResolveDependenciesFromRegistries( const isBuiltIn = await checkIsBuiltInPackage(packageName, version); if (isBuiltIn) { graph = markBuiltInResolved(graph, packageName, version); - return graph; + return await resolveRecursively(graph, sources, deep); } const errors: Record = {}; @@ -95,7 +95,7 @@ export function ResolveDependenciesFromRegistries( } graph = markFailed(graph, packageName, version, errors); - return graph; + return await resolveRecursively(graph, sources, deep); } return (sources, packageName, version, deep) => diff --git a/test/e2e/add.test.ts b/test/e2e/add.test.ts index 2c732088..4b41bbcb 100644 --- a/test/e2e/add.test.ts +++ b/test/e2e/add.test.ts @@ -1,10 +1,10 @@ -import { prepareHomeDirectory } from "./setup/directories"; -import { runOpenupm } from "./run"; -import { prepareUnityProject } from "./setup/project"; import { ResultCodes } from "../../src/cli/result-codes"; -import { getProjectManifest } from "./check/project-manifest"; import { emptyProjectManifest } from "../../src/domain/project-manifest"; import { RegistryUrl } from "../../src/domain/registry-url"; +import { getProjectManifest } from "./check/project-manifest"; +import { runOpenupm } from "./run"; +import { prepareHomeDirectory } from "./setup/directories"; +import { prepareUnityProject } from "./setup/project"; describe("add packages", () => { type SuccessfulAddCase = { @@ -109,6 +109,23 @@ describe("add packages", () => { ); }); + it("should add package with both openupm and unity dependencies", async () => { + // See https://github.com/openupm/openupm-cli/issues/392 + await testSuccessfulAdd( + [ + { + packageName: "com.realitycollective.service-framework", + addVersion: "1.0.8", + expectedVersion: "1.0.8", + }, + ], + [ + "com.realitycollective.service-framework", + "com.realitycollective.utilities", + ] + ); + }); + it("should add multiple packages", async () => { await testSuccessfulAdd( [ From 4c421fe4c5bf95ae809e66a63c5a3686e7921d15 Mon Sep 17 00:00:00 2001 From: semantic-release-bot Date: Sat, 7 Sep 2024 09:03:25 +0000 Subject: [PATCH 17/17] chore(release): 4.1.2 [skip ci] ## [4.1.2](https://github.com/openupm/openupm-cli/compare/4.1.1...4.1.2) (2024-09-07) ### Bug Fixes * partially resolved dependencies ([#393](https://github.com/openupm/openupm-cli/issues/393)) ([13b5577](https://github.com/openupm/openupm-cli/commit/13b557750fc54a56aad21337055f72a0e275a82f)) --- CHANGELOG.md | 7 +++++++ package-lock.json | 4 ++-- package.json | 2 +- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6cab8326..0b92ae15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,10 @@ +## [4.1.2](https://github.com/openupm/openupm-cli/compare/4.1.1...4.1.2) (2024-09-07) + + +### Bug Fixes + +* partially resolved dependencies ([#393](https://github.com/openupm/openupm-cli/issues/393)) ([13b5577](https://github.com/openupm/openupm-cli/commit/13b557750fc54a56aad21337055f72a0e275a82f)) + ## [4.1.1](https://github.com/openupm/openupm-cli/compare/4.1.0...4.1.1) (2024-08-31) diff --git a/package-lock.json b/package-lock.json index 156d0633..5c006e20 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "openupm-cli", - "version": "4.1.1", + "version": "4.1.2", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "openupm-cli", - "version": "4.1.1", + "version": "4.1.2", "license": "BSD-3-Clause", "dependencies": { "@commander-js/extra-typings": "^9.5.0", diff --git a/package.json b/package.json index 25d7a8b5..8dac14a9 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "openupm-cli", - "version": "4.1.1", + "version": "4.1.2", "preferGlobal": true, "description": "openupm command line interface", "engines": {