From 430a43401a0f45de9810d7db67427708a71ea573 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Tue, 9 Jul 2024 09:43:18 +0200 Subject: [PATCH 1/8] fix: 371 error cannot find module node fetch (#372) * deps: add node-fetch * deps: remove node-fetch types Seems like node-fetch already includes types * deps: downgrade node-fetch Seems like node-fetch@3 requires esm. https://stackoverflow.com/questions/69087292/requirenode-fetch-gives-err-require-esm The solution is to use @2. --- package-lock.json | 34 +++++++++++++++++++--------------- package.json | 2 +- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/package-lock.json b/package-lock.json index f3196bec..a82f0b42 100644 --- a/package-lock.json +++ b/package-lock.json @@ -19,6 +19,7 @@ "is-wsl": "^2.2.0", "libnpmsearch": "^5.0.3", "mkdirp": "^3.0.1", + "node-fetch": "^2.7.0", "npm-registry-fetch": "^13.1.1", "npmlog": "^6.0.2", "pkginfo": "^0.4.1", @@ -45,7 +46,6 @@ "@types/jest": "^29.5.12", "@types/libnpmsearch": "^2.0.4", "@types/node": "^18.19.31", - "@types/node-fetch": "^2.6.9", "@types/npmlog": "^4.1.4", "@types/pkginfo": "^0.4.1", "@types/promptly": "^3.0.3", @@ -7092,9 +7092,9 @@ } }, "node_modules/node-fetch": { - "version": "2.6.7", - "dev": true, - "license": "MIT", + "version": "2.7.0", + "resolved": "https://registry.npmjs.org/node-fetch/-/node-fetch-2.7.0.tgz", + "integrity": "sha512-c4FRfUm/dbcWZ7U+1Wq0AwCyFL+3nt2bEw05wfxSz+DWpWsitgmSgYmy2dQdWyKC1694ELPqMs/YzUSNozLt8A==", "dependencies": { "whatwg-url": "^5.0.0" }, @@ -11685,8 +11685,8 @@ }, "node_modules/tr46": { "version": "0.0.3", - "dev": true, - "license": "MIT" + "resolved": "https://registry.npmjs.org/tr46/-/tr46-0.0.3.tgz", + "integrity": "sha512-N3WMsuqV66lT30CrXNbEjx4GEwlow3v6rr4mCcv6prnfwhS01rkgyFdjPNBYd9br7LpXV1+Emh01fHnq2Gdgrw==" }, "node_modules/traverse": { "version": "0.6.6", @@ -12049,13 +12049,13 @@ }, "node_modules/webidl-conversions": { "version": "3.0.1", - "dev": true, - "license": "BSD-2-Clause" + "resolved": "https://registry.npmjs.org/webidl-conversions/-/webidl-conversions-3.0.1.tgz", + "integrity": "sha512-2JAn3z8AR6rjK8Sm8orRC0h/bcl/DqL7tRPdGZ4I1CjdF+EaMLmYxBHyXuKL849eucPFhvBoxMsflfOb8kxaeQ==" }, "node_modules/whatwg-url": { "version": "5.0.0", - "dev": true, - "license": "MIT", + "resolved": "https://registry.npmjs.org/whatwg-url/-/whatwg-url-5.0.0.tgz", + "integrity": "sha512-saE57nupxk6v3HY35+jzBwYa0rKSy0XR8JSxZPwgLr7ys0IBzhGviA1/TUGJLmSVqs8pb9AnvICXEuOHLprYTw==", "dependencies": { "tr46": "~0.0.3", "webidl-conversions": "^3.0.0" @@ -17042,8 +17042,9 @@ } }, "node-fetch": { - "version": "2.6.7", - "dev": true, + "version": "2.7.0", + "resolved": "https://registry.npmjs.org/node-fetch/-/node-fetch-2.7.0.tgz", + "integrity": "sha512-c4FRfUm/dbcWZ7U+1Wq0AwCyFL+3nt2bEw05wfxSz+DWpWsitgmSgYmy2dQdWyKC1694ELPqMs/YzUSNozLt8A==", "requires": { "whatwg-url": "^5.0.0" } @@ -20144,7 +20145,8 @@ }, "tr46": { "version": "0.0.3", - "dev": true + "resolved": "https://registry.npmjs.org/tr46/-/tr46-0.0.3.tgz", + "integrity": "sha512-N3WMsuqV66lT30CrXNbEjx4GEwlow3v6rr4mCcv6prnfwhS01rkgyFdjPNBYd9br7LpXV1+Emh01fHnq2Gdgrw==" }, "traverse": { "version": "0.6.6", @@ -20372,11 +20374,13 @@ }, "webidl-conversions": { "version": "3.0.1", - "dev": true + "resolved": "https://registry.npmjs.org/webidl-conversions/-/webidl-conversions-3.0.1.tgz", + "integrity": "sha512-2JAn3z8AR6rjK8Sm8orRC0h/bcl/DqL7tRPdGZ4I1CjdF+EaMLmYxBHyXuKL849eucPFhvBoxMsflfOb8kxaeQ==" }, "whatwg-url": { "version": "5.0.0", - "dev": true, + "resolved": "https://registry.npmjs.org/whatwg-url/-/whatwg-url-5.0.0.tgz", + "integrity": "sha512-saE57nupxk6v3HY35+jzBwYa0rKSy0XR8JSxZPwgLr7ys0IBzhGviA1/TUGJLmSVqs8pb9AnvICXEuOHLprYTw==", "requires": { "tr46": "~0.0.3", "webidl-conversions": "^3.0.0" diff --git a/package.json b/package.json index ea80162a..8ebca973 100644 --- a/package.json +++ b/package.json @@ -42,7 +42,6 @@ "@types/jest": "^29.5.12", "@types/libnpmsearch": "^2.0.4", "@types/node": "^18.19.31", - "@types/node-fetch": "^2.6.9", "@types/npmlog": "^4.1.4", "@types/pkginfo": "^0.4.1", "@types/promptly": "^3.0.3", @@ -73,6 +72,7 @@ "is-wsl": "^2.2.0", "libnpmsearch": "^5.0.3", "mkdirp": "^3.0.1", + "node-fetch": "^2.7.0", "npm-registry-fetch": "^13.1.1", "npmlog": "^6.0.2", "pkginfo": "^0.4.1", From d7c105387fdc0e21e0dd99210571f54201439f59 Mon Sep 17 00:00:00 2001 From: semantic-release-bot Date: Tue, 9 Jul 2024 07:45:59 +0000 Subject: [PATCH 2/8] chore(release): 3.3.1 [skip ci] ## [3.3.1](https://github.com/openupm/openupm-cli/compare/3.3.0...3.3.1) (2024-07-09) ### Bug Fixes * 371 error cannot find module node fetch ([#372](https://github.com/openupm/openupm-cli/issues/372)) ([430a434](https://github.com/openupm/openupm-cli/commit/430a43401a0f45de9810d7db67427708a71ea573)) --- 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 6b4e32f3..99c64e58 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,10 @@ +## [3.3.1](https://github.com/openupm/openupm-cli/compare/3.3.0...3.3.1) (2024-07-09) + + +### Bug Fixes + +* 371 error cannot find module node fetch ([#372](https://github.com/openupm/openupm-cli/issues/372)) ([430a434](https://github.com/openupm/openupm-cli/commit/430a43401a0f45de9810d7db67427708a71ea573)) + # [3.3.0](https://github.com/openupm/openupm-cli/compare/3.2.0...3.3.0) (2024-07-05) diff --git a/package-lock.json b/package-lock.json index a82f0b42..fcfe8720 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "openupm-cli", - "version": "3.3.0", + "version": "3.3.1", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "openupm-cli", - "version": "3.3.0", + "version": "3.3.1", "license": "BSD-3-Clause", "dependencies": { "@commander-js/extra-typings": "^9.5.0", diff --git a/package.json b/package.json index 8ebca973..4c06a8b4 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "openupm-cli", - "version": "3.3.0", + "version": "3.3.1", "preferGlobal": true, "description": "openupm command line interface", "engines": { From 2416da69011597fa2ae27cb5a61f485239a77924 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Sun, 7 Jul 2024 15:56:54 +0200 Subject: [PATCH 3/8] refactor: dependency resolving logic Use deps graph --- src/cli/cmd-add.ts | 51 ++++-- src/cli/cmd-deps.ts | 35 ++-- src/cli/dependency-logging.ts | 13 +- src/cli/index.ts | 2 +- src/services/dependency-resolving.ts | 262 +++++++++++---------------- test/cli/cmd-add.test.ts | 57 +++--- test/cli/cmd-deps.test.ts | 59 +++--- 7 files changed, 216 insertions(+), 263 deletions(-) diff --git a/src/cli/cmd-add.ts b/src/cli/cmd-add.ts index 3780fc5c..e38cae07 100644 --- a/src/cli/cmd-add.ts +++ b/src/cli/cmd-add.ts @@ -37,6 +37,7 @@ import { DebugLog } from "../logging"; import { DetermineEditorVersion } from "../services/determine-editor-version"; import { ResultCodes } from "./result-codes"; import { logError } from "./error-logging"; +import { recordEntries } from "../utils/record-utils"; export class PackageIncompatibleError extends CustomError { constructor( @@ -174,35 +175,45 @@ export function makeAddCmd( // pkgsInScope if (!isUpstreamPackage) { debugLog(`fetch: ${makePackageReference(name, requestedVersion)}`); - const [depsValid, depsInvalid] = await resolveDependencies( + const dependencyGraph = await resolveDependencies( [env.registry, env.upstreamRegistry], name, versionToAdd, true ); - // add depsValid to pkgsInScope. - depsValid.forEach((dependency) => - logValidDependency(debugLog, dependency) + let isAnyDependencyUnresolved = false; + recordEntries(dependencyGraph).forEach(([dependencyName, versions]) => + recordEntries(versions).forEach( + ([dependencyVersion, dependency]) => { + if (!dependency.resolved) { + logError(log, dependency.error); + // If the manifest already has the dependency than it does not + // really matter that it was not resolved. + if (!hasDependency(manifest, dependencyName)) + isAnyDependencyUnresolved = true; + return; + } + + const dependencyRef = makePackageReference( + dependencyName, + dependencyVersion + ); + logValidDependency(debugLog, dependencyRef, dependency.source); + + const isUnityPackage = + dependency.source === "built-in" || + dependency.source === unityRegistryUrl; + if (isUnityPackage) return; + + // add depsValid to pkgsInScope. + pkgsInScope.push(dependencyName); + } + ) ); - depsValid - .filter((x) => { - const isUnityPackage = - x.source === "built-in" || x.source === unityRegistryUrl; - return !isUnityPackage; - }) - .map((x) => x.name) - .forEach((name) => pkgsInScope.push(name)); + // print suggestion for depsInvalid - let isAnyDependencyUnresolved = false; - depsInvalid.forEach((depObj) => { - logError(log, depObj.reason); - // If the manifest already has the dependency than it does not - // really matter that it was not resolved. - if (!hasDependency(manifest, depObj.name)) - isAnyDependencyUnresolved = true; - }); if (isAnyDependencyUnresolved && !options.force) throw new UnresolvedDependenciesError( makePackageReference(name, versionToAdd) diff --git a/src/cli/cmd-deps.ts b/src/cli/cmd-deps.ts index 7a4a0d2d..5484f0da 100644 --- a/src/cli/cmd-deps.ts +++ b/src/cli/cmd-deps.ts @@ -16,6 +16,7 @@ import { DebugLog } from "../logging"; import { ResultCodes } from "./result-codes"; import { ResolveLatestVersion } from "../services/resolve-latest-version"; import { isSemanticVersion } from "../domain/semantic-version"; +import { recordEntries } from "../utils/record-utils"; export type DepsOptions = CmdOptions<{ deep?: boolean; @@ -76,25 +77,33 @@ export function makeDepsCmd( debugLog( `fetch: ${makePackageReference(packageName, latestVersion)}, deep=${deep}` ); - const [depsValid, depsInvalid] = await resolveDependencies( + const dependencyGraph = await resolveDependencies( sources, packageName, latestVersion, deep ); - depsValid.forEach((dependency) => logValidDependency(debugLog, dependency)); - depsValid - .filter((x) => !x.self) - .forEach((x) => - log.notice("dependency", `${makePackageReference(x.name, x.version)}`) - ); - depsInvalid - .filter((x) => !x.self) - .forEach((x) => { - const prefix = errorPrefixForError(x.reason); - log.warn(prefix, x.name); - }); + recordEntries(dependencyGraph).forEach(([dependencyName, versions]) => + recordEntries(versions).forEach(([dependencyVersion, dependency]) => { + if (!dependency.resolved) { + if (dependencyName !== packageName) { + const prefix = errorPrefixForError(dependency.error); + log.warn(prefix, dependencyName); + } + return; + } + const dependencyRef = makePackageReference( + dependencyName, + dependencyVersion + ); + logValidDependency(debugLog, dependencyRef, dependency.source); + + if (dependencyName === packageName) return; + + log.notice("dependency", `${dependencyRef}`); + }) + ); return ResultCodes.Ok; }; diff --git a/src/cli/dependency-logging.ts b/src/cli/dependency-logging.ts index c9563ca5..cfc378c6 100644 --- a/src/cli/dependency-logging.ts +++ b/src/cli/dependency-logging.ts @@ -1,6 +1,5 @@ -import { ValidDependency } from "../services/dependency-resolving"; -import { makePackageReference } from "../domain/package-reference"; -import { unityRegistryUrl } from "../domain/registry-url"; +import { PackageReference } from "../domain/package-reference"; +import { RegistryUrl, unityRegistryUrl } from "../domain/registry-url"; import { DebugLog } from "../logging"; /** @@ -8,13 +7,13 @@ import { DebugLog } from "../logging"; */ export function logValidDependency( debugLog: DebugLog, - dependency: ValidDependency + packageRef: PackageReference, + source: RegistryUrl | "built-in" ) { - const packageRef = makePackageReference(dependency.name, dependency.version); const tag = - dependency.source === "built-in" + source === "built-in" ? "[internal] " - : dependency.source === unityRegistryUrl + : source === unityRegistryUrl ? "[upstream]" : ""; const message = `${packageRef} ${tag}`; diff --git a/src/cli/index.ts b/src/cli/index.ts index 1c264961..c2a2ab9c 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -110,7 +110,7 @@ const checkIsBuiltInPackage = makeCheckIsBuiltInPackage( fetchPackument ); const resolveDependencies = makeResolveDependency( - resolveRemovePackumentVersion, + fetchPackument, checkIsBuiltInPackage ); const saveAuthToUpmConfig = makeSaveAuthToUpmConfig( diff --git a/src/services/dependency-resolving.ts b/src/services/dependency-resolving.ts index 5eccb88a..de5af29f 100644 --- a/src/services/dependency-resolving.ts +++ b/src/services/dependency-resolving.ts @@ -1,192 +1,136 @@ import { DomainName } from "../domain/domain-name"; import { SemanticVersion } from "../domain/semantic-version"; -import { addToCache, emptyPackumentCache } from "../packument-cache"; -import { - pickMostFixable, - ResolvableVersion, - ResolvedPackumentVersion, - ResolvePackumentVersionError, - tryResolveFromCache, -} from "../packument-version-resolving"; -import { RegistryUrl } from "../domain/registry-url"; +import { ResolvePackumentVersionError } from "../packument-version-resolving"; import { Registry } from "../domain/registry"; -import { ResolveRemotePackumentVersion } from "./resolve-remote-packument-version"; -import { areArraysEqual } from "../utils/array-utils"; -import { dependenciesOf } from "../domain/package-manifest"; -import { Err, Result } from "ts-results-es"; -import { PackumentNotFoundError } from "../common-errors"; import { CheckIsBuiltInPackage } from "./built-in-package-check"; +import { RegistryUrl } from "../domain/registry-url"; +import { tryResolvePackumentVersion } from "../domain/packument"; +import { FetchPackument } from "../io/packument-io"; +import { PackumentNotFoundError } from "../common-errors"; +import { recordEntries } from "../utils/record-utils"; -export type DependencyBase = { - /** - * The package name of the dependency. - */ - readonly name: DomainName; - /** - * Whether this dependency is the root package for which dependencies were - * requested. - */ - readonly self: boolean; -}; +type NameVersionPair = Readonly<[DomainName, SemanticVersion]>; -/** - * A dependency that was resolved successfully. - */ -export interface ValidDependency extends DependencyBase { - /** - * The source from which this dependency was resolved. Either the url of the - * source registry or "built-in" if the dependency was a built-in package. - */ - readonly source: RegistryUrl | "built-in"; - /** - * The requested version. - */ - readonly version: SemanticVersion; -} +type ResolvedNode = Readonly<{ + resolved: true; + source: RegistryUrl | "built-in"; + dependencies: Readonly>; +}>; -/** - * A dependency that could not be resolved. - */ -export interface InvalidDependency extends DependencyBase { - reason: ResolvePackumentVersionError; +type UnresolvedNode = Readonly<{ + resolved: false; + error: ResolvePackumentVersionError; +}>; + +type GraphNode = ResolvedNode | UnresolvedNode; + +type DependencyGraph = Readonly< + Record>> +>; + +const emptyGraph: DependencyGraph = {}; + +function setGraphNode( + graph: DependencyGraph, + packageName: DomainName, + version: SemanticVersion, + node: GraphNode +): DependencyGraph { + return { + ...graph, + [packageName]: { ...graph[packageName], [version]: node }, + }; } -type NameVersionPair = Readonly<[DomainName, SemanticVersion]>; +function graphHasNode( + graph: DependencyGraph, + packageName: DomainName, + version: SemanticVersion +): boolean { + return graph[packageName]?.[version] !== undefined; +} /** * Function for resolving all dependencies for a package. * @param sources Sources from which dependencies can be resolved. - * @param name The name of the package. + * @param packageName The name of the package. * @param version The version for which to search dependencies. * @param deep Whether to search for all dependencies. */ export type ResolveDependencies = ( sources: ReadonlyArray, - name: DomainName, + packageName: DomainName, version: SemanticVersion, deep: boolean -) => Promise<[ValidDependency[], InvalidDependency[]]>; +) => Promise; /** * Makes a {@link ResolveDependencies} function. */ export function makeResolveDependency( - resolveRemovePackumentVersion: ResolveRemotePackumentVersion, + fetchPackument: FetchPackument, checkIsBuiltInPackage: CheckIsBuiltInPackage ): ResolveDependencies { - // TODO: Add tests for this service - - return async (sources, name, version, deep) => { - // a list of pending dependency {name, version} - const pendingList = Array.of([name, version]); - // a list of processed dependency {name, version} - const processedList = Array.of(); - // a list of dependency entry exists on the registry - const depsValid = Array.of(); - // a list of dependency entry doesn't exist on the registry - const depsInvalid = Array.of(); - // cached dict - let packumentCache = emptyPackumentCache; - - async function tryResolveFromRegistry( - registry: Registry, - packumentName: DomainName, - version: ResolvableVersion - ) { - // First try cache - const cacheResult = tryResolveFromCache( - packumentCache, - registry.url, - packumentName, - version + async function resolveRecursively( + graph: DependencyGraph, + sources: ReadonlyArray, + packagesToCheck: ReadonlyArray, + deep: boolean + ): Promise { + if (packagesToCheck.length === 0) return graph; + + const [packageName, version] = packagesToCheck[0]!; + if (graphHasNode(graph, packageName, version)) + return await resolveRecursively( + graph, + sources, + packagesToCheck.slice(1), + deep ); - if (cacheResult.isOk()) return cacheResult; - - // Then registry - return await resolveRemovePackumentVersion( - packumentName, - version, - registry - ).promise; - } - while (pendingList.length > 0) { - // NOTE: Guaranteed defined because of while loop logic - const entry = pendingList.shift()!; - const [entryName, entryVersion] = entry; + const isBuiltIn = await checkIsBuiltInPackage(packageName, version); + if (isBuiltIn) + return setGraphNode(graph, packageName, version, { + resolved: true, + source: "built-in", + dependencies: {}, + }); - const isProcessed = processedList.some((processed) => - areArraysEqual(processed, entry) + for (const source of sources) { + const packument = await fetchPackument(source, packageName); + if (packument === null) continue; + + const packumentVersionResult = tryResolvePackumentVersion( + packument, + version + ); + if (packumentVersionResult.isErr()) + return setGraphNode(graph, packageName, version, { + resolved: false, + error: packumentVersionResult.error, + }); + const dependencies = packumentVersionResult.value.dependencies ?? {}; + const updatedGraph = setGraphNode(graph, packageName, version, { + resolved: true, + source: source.url, + dependencies, + }); + if (!deep) return updatedGraph; + + return await resolveRecursively( + graph, + sources, + [...packagesToCheck.slice(1), ...recordEntries(dependencies)], + deep ); - if (!isProcessed) { - // add entry to processed list - processedList.push(entry); - const isInternal = await checkIsBuiltInPackage(entryName, entryVersion); - const isSelf = entryName === name; - - if (isInternal) { - depsValid.push({ - name: entryName, - version: entryVersion, - source: "built-in", - self: isSelf, - }); - continue; - } - - // Search all given registries. - let resolveResult: Result< - ResolvedPackumentVersion, - ResolvePackumentVersionError - > = Err(new PackumentNotFoundError(entryName)); - for (const source of sources) { - const result = await tryResolveFromRegistry( - source, - entryName, - entryVersion - ); - if (result.isErr()) { - resolveResult = pickMostFixable(resolveResult, result); - continue; - } - - resolveResult = result; - break; - } - - if (resolveResult.isErr()) { - depsInvalid.push({ - name: entryName, - self: isSelf, - reason: resolveResult.error, - }); - continue; - } - - // Packument was resolved successfully - const source = resolveResult.value.source; - const resolvedPackumentVersion = resolveResult.value.packumentVersion; - packumentCache = addToCache( - packumentCache, - resolveResult.value.source, - resolveResult.value.packument - ); - - // add dependencies to pending list - if (isSelf || deep) { - const dependencies = dependenciesOf(resolvedPackumentVersion); - pendingList.push(...dependencies); - } - - const dependency: ValidDependency = { - name: entryName, - version: entryVersion, - source, - self: isSelf, - }; - depsValid.push(dependency); - } } - return [depsValid, depsInvalid]; - }; + + return setGraphNode(graph, packageName, version, { + resolved: false, + error: new PackumentNotFoundError(packageName), + }); + } + + return (sources, packageName, version, deep) => + resolveRecursively(emptyGraph, sources, [[packageName, version]], deep); } diff --git a/test/cli/cmd-add.test.ts b/test/cli/cmd-add.test.ts index 70c3705d..f6b843fc 100644 --- a/test/cli/cmd-add.test.ts +++ b/test/cli/cmd-add.test.ts @@ -74,23 +74,22 @@ function makeDependencies() { ); const resolveDependencies = mockService(); - resolveDependencies.mockResolvedValue([ - [ - { - name: somePackage, - version: makeSemanticVersion("1.0.0"), + resolveDependencies.mockResolvedValue({ + [somePackage]: { + [makeSemanticVersion("1.0.0")]: { + resolved: true, source: exampleRegistryUrl, - self: true, + dependencies: { [otherPackage]: makeSemanticVersion("1.0.0") }, }, - { - name: otherPackage, - version: makeSemanticVersion("1.0.0"), + }, + [otherPackage]: { + [makeSemanticVersion("1.0.0")]: { + resolved: true, source: exampleRegistryUrl, - self: false, + dependencies: {}, }, - ], - [], - ]); + }, + }); const loadProjectManifest = mockService(); loadProjectManifest.mockResolvedValue(emptyProjectManifest); @@ -235,20 +234,18 @@ describe("cmd-add", () => { it("should fail if dependency could not be resolved and not running with force", async () => { const { addCmd, resolveDependencies } = makeDependencies(); - resolveDependencies.mockResolvedValue([ - [], - [ - { - name: otherPackage, - self: false, - reason: new VersionNotFoundError( + resolveDependencies.mockResolvedValue({ + [otherPackage]: { + [makeSemanticVersion("1.0.0")]: { + resolved: false, + error: new VersionNotFoundError( otherPackage, makeSemanticVersion("1.0.0"), [] ), }, - ], - ]); + }, + }); await expect(() => addCmd(somePackage, { @@ -259,20 +256,18 @@ describe("cmd-add", () => { it("should add package with unresolved dependency when running with force", async () => { const { addCmd, resolveDependencies } = makeDependencies(); - resolveDependencies.mockResolvedValue([ - [], - [ - { - name: otherPackage, - self: false, - reason: new VersionNotFoundError( + resolveDependencies.mockResolvedValue({ + [otherPackage]: { + [makeSemanticVersion("1.0.0")]: { + resolved: false, + error: new VersionNotFoundError( otherPackage, makeSemanticVersion("1.0.0"), [] ), }, - ], - ]); + }, + }); const resultCode = await addCmd(somePackage, { _global: {}, diff --git a/test/cli/cmd-deps.test.ts b/test/cli/cmd-deps.test.ts index 11d0c8eb..f70a3595 100644 --- a/test/cli/cmd-deps.test.ts +++ b/test/cli/cmd-deps.test.ts @@ -9,10 +9,10 @@ import { makeSemanticVersion } from "../../src/domain/semantic-version"; import { PackumentNotFoundError } from "../../src/common-errors"; import { ResolveDependencies } from "../../src/services/dependency-resolving"; import { mockService } from "../services/service.mock"; -import { VersionNotFoundError } from "../../src/domain/packument"; import { noopLogger } from "../../src/logging"; import { ResultCodes } from "../../src/cli/result-codes"; import { ResolveLatestVersion } from "../../src/services/resolve-latest-version"; +import { VersionNotFoundError } from "../../src/domain/packument"; const somePackage = makeDomainName("com.some.package"); const otherPackage = makeDomainName("com.other.package"); @@ -27,23 +27,22 @@ function makeDependencies() { parseEnv.mockResolvedValue(defaultEnv); const resolveDependencies = mockService(); - resolveDependencies.mockResolvedValue([ - [ - { + resolveDependencies.mockResolvedValue({ + [somePackage]: { + [makeSemanticVersion("1.2.3")]: { + resolved: true, source: exampleRegistryUrl, - self: true, - name: somePackage, - version: makeSemanticVersion("1.2.3"), + dependencies: { [otherPackage]: makeSemanticVersion("1.2.3") }, }, - { + }, + [otherPackage]: { + [makeSemanticVersion("1.2.3")]: { + resolved: true, source: exampleRegistryUrl, - self: false, - name: otherPackage, - version: makeSemanticVersion("1.2.3"), + dependencies: {}, }, - ], - [], - ]); + }, + }); const resolveLatestVersion = mockService(); resolveLatestVersion.mockResolvedValue({ @@ -125,16 +124,14 @@ describe("cmd-deps", () => { it("should log missing dependency", async () => { const { depsCmd, resolveDependencies, log } = makeDependencies(); - resolveDependencies.mockResolvedValue([ - [], - [ - { - name: otherPackage, - self: false, - reason: new PackumentNotFoundError(otherPackage), + resolveDependencies.mockResolvedValue({ + [otherPackage]: { + [makeSemanticVersion("1.2.3")]: { + resolved: false, + error: new PackumentNotFoundError(otherPackage), }, - ], - ]); + }, + }); await depsCmd(somePackage, { _global: {}, @@ -148,20 +145,18 @@ describe("cmd-deps", () => { it("should log missing dependency version", async () => { const { depsCmd, resolveDependencies, log } = makeDependencies(); - resolveDependencies.mockResolvedValue([ - [], - [ - { - name: otherPackage, - self: false, - reason: new VersionNotFoundError( + resolveDependencies.mockResolvedValue({ + [otherPackage]: { + [makeSemanticVersion("1.2.3")]: { + resolved: false, + error: new VersionNotFoundError( otherPackage, makeSemanticVersion("1.2.3"), [] ), }, - ], - ]); + }, + }); await depsCmd(somePackage, { _global: {}, From ffb65616b1e857da909b4b15e079e514ea60b881 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Sun, 7 Jul 2024 16:13:37 +0200 Subject: [PATCH 4/8] refactor: extract type Move dependency-graph and related logic to own module. Also add tests. --- src/domain/dependency-graph.ts | 79 +++++++++++++++++++++++++++ src/services/dependency-resolving.ts | 51 +++-------------- test/domain/deppendency-graph.test.ts | 78 ++++++++++++++++++++++++++ 3 files changed, 165 insertions(+), 43 deletions(-) create mode 100644 src/domain/dependency-graph.ts create mode 100644 test/domain/deppendency-graph.test.ts diff --git a/src/domain/dependency-graph.ts b/src/domain/dependency-graph.ts new file mode 100644 index 00000000..39f246b3 --- /dev/null +++ b/src/domain/dependency-graph.ts @@ -0,0 +1,79 @@ +import { ResolvePackumentVersionError } from "../packument-version-resolving"; +import { DomainName } from "./domain-name"; +import { SemanticVersion } from "./semantic-version"; +import { RegistryUrl } from "./registry-url"; + +type ResolvedNode = Readonly<{ + resolved: true; + source: RegistryUrl | "built-in"; + dependencies: Readonly>; +}>; +type UnresolvedNode = Readonly<{ + resolved: false; + error: ResolvePackumentVersionError; +}>; +type GraphNode = ResolvedNode | UnresolvedNode; + +/** + * A graph indicating which packages depend on which others. + * Dependencies are keyed first by their name and then version. + * @example + * { + * "com.some.package": { + * "1.0.0": { + * resolved: true, + * source: "https://package.openupm.com", + * dependencies: { "com.other.package": "2.0.0" } + * } + * }, + * "com.other.package": { + * "2.0.0": { + * resolved: true, + * source: "https://package.openupm.com", + * dependencies: { } + * } + * } + * } + */ +export type DependencyGraph = Readonly< + Record>> +>; + +/** + * An empty dependency graph. + */ +export const emptyDependencyGraph: DependencyGraph = {}; + +/** + * Add or replace a node in the graph. + * @param graph The graph. + * @param packageName The key package name. + * @param version The key version. + * @param node The new node. + * @returns The updated graph. + */ +export function setGraphNode( + graph: DependencyGraph, + packageName: DomainName, + version: SemanticVersion, + node: GraphNode +): DependencyGraph { + return { + ...graph, + [packageName]: { ...graph[packageName], [version]: node }, + }; +} + +/** + * Checks whether a graph has a node at the given key. + * @param graph The graph. + * @param packageName The key package name. + * @param version The key version. + */ +export function graphHasNodeAt( + graph: DependencyGraph, + packageName: DomainName, + version: SemanticVersion +): boolean { + return graph[packageName]?.[version] !== undefined; +} diff --git a/src/services/dependency-resolving.ts b/src/services/dependency-resolving.ts index de5af29f..0f2424a9 100644 --- a/src/services/dependency-resolving.ts +++ b/src/services/dependency-resolving.ts @@ -1,55 +1,20 @@ import { DomainName } from "../domain/domain-name"; import { SemanticVersion } from "../domain/semantic-version"; -import { ResolvePackumentVersionError } from "../packument-version-resolving"; import { Registry } from "../domain/registry"; import { CheckIsBuiltInPackage } from "./built-in-package-check"; -import { RegistryUrl } from "../domain/registry-url"; import { tryResolvePackumentVersion } from "../domain/packument"; import { FetchPackument } from "../io/packument-io"; import { PackumentNotFoundError } from "../common-errors"; import { recordEntries } from "../utils/record-utils"; +import { + DependencyGraph, + emptyDependencyGraph, + graphHasNodeAt, + setGraphNode, +} from "../domain/dependency-graph"; type NameVersionPair = Readonly<[DomainName, SemanticVersion]>; -type ResolvedNode = Readonly<{ - resolved: true; - source: RegistryUrl | "built-in"; - dependencies: Readonly>; -}>; - -type UnresolvedNode = Readonly<{ - resolved: false; - error: ResolvePackumentVersionError; -}>; - -type GraphNode = ResolvedNode | UnresolvedNode; - -type DependencyGraph = Readonly< - Record>> ->; - -const emptyGraph: DependencyGraph = {}; - -function setGraphNode( - graph: DependencyGraph, - packageName: DomainName, - version: SemanticVersion, - node: GraphNode -): DependencyGraph { - return { - ...graph, - [packageName]: { ...graph[packageName], [version]: node }, - }; -} - -function graphHasNode( - graph: DependencyGraph, - packageName: DomainName, - version: SemanticVersion -): boolean { - return graph[packageName]?.[version] !== undefined; -} - /** * Function for resolving all dependencies for a package. * @param sources Sources from which dependencies can be resolved. @@ -80,7 +45,7 @@ export function makeResolveDependency( if (packagesToCheck.length === 0) return graph; const [packageName, version] = packagesToCheck[0]!; - if (graphHasNode(graph, packageName, version)) + if (graphHasNodeAt(graph, packageName, version)) return await resolveRecursively( graph, sources, @@ -132,5 +97,5 @@ export function makeResolveDependency( } return (sources, packageName, version, deep) => - resolveRecursively(emptyGraph, sources, [[packageName, version]], deep); + resolveRecursively(emptyDependencyGraph, sources, [[packageName, version]], deep); } diff --git a/test/domain/deppendency-graph.test.ts b/test/domain/deppendency-graph.test.ts new file mode 100644 index 00000000..d28fc24f --- /dev/null +++ b/test/domain/deppendency-graph.test.ts @@ -0,0 +1,78 @@ +import { + emptyDependencyGraph, + graphHasNodeAt, + setGraphNode, +} from "../../src/domain/dependency-graph"; +import { makeDomainName } from "../../src/domain/domain-name"; +import { makeSemanticVersion } from "../../src/domain/semantic-version"; +import { exampleRegistryUrl } from "./data-registry"; +import { PackumentNotFoundError } from "../../src/common-errors"; + +describe("dependency graph", () => { + const somePackage = makeDomainName("com.some.package"); + const someVersion = makeSemanticVersion("1.0.0"); + + describe("set node", () => { + it("should add new node", () => { + const initial = emptyDependencyGraph; + const node = { + resolved: true, + source: exampleRegistryUrl, + dependencies: {}, + } as const; + + const withNode = setGraphNode(initial, somePackage, someVersion, node); + + expect(withNode).toEqual({ [somePackage]: { [someVersion]: node } }); + }); + + it("should overwrite node", () => { + const initial = setGraphNode( + emptyDependencyGraph, + somePackage, + someVersion, + { + resolved: false, + error: new PackumentNotFoundError(somePackage), + } + ); + const node = { + resolved: true, + source: exampleRegistryUrl, + dependencies: {}, + } as const; + + const withNode = setGraphNode(initial, somePackage, someVersion, node); + + expect(withNode).toEqual({ [somePackage]: { [someVersion]: node } }); + }); + }); + + describe("has node", () => { + it("should be false if node was not added", () => { + const actual = graphHasNodeAt( + emptyDependencyGraph, + somePackage, + someVersion + ); + + expect(actual).toBeFalsy(); + }); + + it("should be true if node was added", () => { + const initial = setGraphNode( + emptyDependencyGraph, + somePackage, + someVersion, + { + resolved: false, + error: new PackumentNotFoundError(somePackage), + } + ); + + const actual = graphHasNodeAt(initial, somePackage, someVersion); + + expect(actual).toBeTruthy(); + }); + }); +}); From b10cd958440b8b798a7c049211e2c4f4f8eb3ee0 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Mon, 8 Jul 2024 17:42:34 +0200 Subject: [PATCH 5/8] fix: typo --- .../{deppendency-graph.test.ts => dependency-graph.test.ts} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename test/domain/{deppendency-graph.test.ts => dependency-graph.test.ts} (100%) diff --git a/test/domain/deppendency-graph.test.ts b/test/domain/dependency-graph.test.ts similarity index 100% rename from test/domain/deppendency-graph.test.ts rename to test/domain/dependency-graph.test.ts From c1dde5c180b24263bf057059da6e28619a18ad9a Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Mon, 8 Jul 2024 17:47:22 +0200 Subject: [PATCH 6/8] refactor: extract utility function Extract function for flattening a dependency graph into a linear array. --- src/cli/cmd-add.ts | 48 +++++++++++++--------------- src/cli/cmd-deps.ts | 8 ++--- src/domain/dependency-graph.ts | 17 ++++++++++ test/domain/dependency-graph.test.ts | 29 +++++++++++++++++ 4 files changed, 73 insertions(+), 29 deletions(-) diff --git a/src/cli/cmd-add.ts b/src/cli/cmd-add.ts index e38cae07..1fc00f3e 100644 --- a/src/cli/cmd-add.ts +++ b/src/cli/cmd-add.ts @@ -37,7 +37,7 @@ import { DebugLog } from "../logging"; import { DetermineEditorVersion } from "../services/determine-editor-version"; import { ResultCodes } from "./result-codes"; import { logError } from "./error-logging"; -import { recordEntries } from "../utils/record-utils"; +import { flattenDependencyGraph } from "../domain/dependency-graph"; export class PackageIncompatibleError extends CustomError { constructor( @@ -183,33 +183,31 @@ export function makeAddCmd( ); let isAnyDependencyUnresolved = false; - recordEntries(dependencyGraph).forEach(([dependencyName, versions]) => - recordEntries(versions).forEach( - ([dependencyVersion, dependency]) => { - if (!dependency.resolved) { - logError(log, dependency.error); - // If the manifest already has the dependency than it does not - // really matter that it was not resolved. - if (!hasDependency(manifest, dependencyName)) - isAnyDependencyUnresolved = true; - return; - } + flattenDependencyGraph(dependencyGraph).forEach( + ([dependencyName, dependencyVersion, dependency]) => { + if (!dependency.resolved) { + logError(log, dependency.error); + // If the manifest already has the dependency than it does not + // really matter that it was not resolved. + if (!hasDependency(manifest, dependencyName)) + isAnyDependencyUnresolved = true; + return; + } - const dependencyRef = makePackageReference( - dependencyName, - dependencyVersion - ); - logValidDependency(debugLog, dependencyRef, dependency.source); + const dependencyRef = makePackageReference( + dependencyName, + dependencyVersion + ); + logValidDependency(debugLog, dependencyRef, dependency.source); - const isUnityPackage = - dependency.source === "built-in" || - dependency.source === unityRegistryUrl; - if (isUnityPackage) return; + const isUnityPackage = + dependency.source === "built-in" || + dependency.source === unityRegistryUrl; + if (isUnityPackage) return; - // add depsValid to pkgsInScope. - pkgsInScope.push(dependencyName); - } - ) + // add depsValid to pkgsInScope. + pkgsInScope.push(dependencyName); + } ); // print suggestion for depsInvalid diff --git a/src/cli/cmd-deps.ts b/src/cli/cmd-deps.ts index 5484f0da..5e26b5ef 100644 --- a/src/cli/cmd-deps.ts +++ b/src/cli/cmd-deps.ts @@ -16,7 +16,7 @@ import { DebugLog } from "../logging"; import { ResultCodes } from "./result-codes"; import { ResolveLatestVersion } from "../services/resolve-latest-version"; import { isSemanticVersion } from "../domain/semantic-version"; -import { recordEntries } from "../utils/record-utils"; +import { flattenDependencyGraph } from "../domain/dependency-graph"; export type DepsOptions = CmdOptions<{ deep?: boolean; @@ -84,8 +84,8 @@ export function makeDepsCmd( deep ); - recordEntries(dependencyGraph).forEach(([dependencyName, versions]) => - recordEntries(versions).forEach(([dependencyVersion, dependency]) => { + flattenDependencyGraph(dependencyGraph).forEach( + ([dependencyName, dependencyVersion, dependency]) => { if (!dependency.resolved) { if (dependencyName !== packageName) { const prefix = errorPrefixForError(dependency.error); @@ -102,7 +102,7 @@ export function makeDepsCmd( if (dependencyName === packageName) return; log.notice("dependency", `${dependencyRef}`); - }) + } ); return ResultCodes.Ok; diff --git a/src/domain/dependency-graph.ts b/src/domain/dependency-graph.ts index 39f246b3..3ed1b470 100644 --- a/src/domain/dependency-graph.ts +++ b/src/domain/dependency-graph.ts @@ -2,6 +2,7 @@ import { ResolvePackumentVersionError } from "../packument-version-resolving"; import { DomainName } from "./domain-name"; import { SemanticVersion } from "./semantic-version"; import { RegistryUrl } from "./registry-url"; +import { recordEntries } from "../utils/record-utils"; type ResolvedNode = Readonly<{ resolved: true; @@ -77,3 +78,19 @@ export function graphHasNodeAt( ): boolean { return graph[packageName]?.[version] !== undefined; } + +/** + * Flattens a dependency tree down into a linear array of entries. + * @param graph The graph to flatten. + * @returns All nodes in the graph in a linear array. + */ +export function flattenDependencyGraph( + graph: DependencyGraph +): ReadonlyArray { + return recordEntries(graph).flatMap(([dependencyName, versions]) => + recordEntries(versions).map( + ([dependencyVersion, dependency]) => + [dependencyName, dependencyVersion, dependency] as const + ) + ); +} diff --git a/test/domain/dependency-graph.test.ts b/test/domain/dependency-graph.test.ts index d28fc24f..cf4126af 100644 --- a/test/domain/dependency-graph.test.ts +++ b/test/domain/dependency-graph.test.ts @@ -1,5 +1,6 @@ import { emptyDependencyGraph, + flattenDependencyGraph, graphHasNodeAt, setGraphNode, } from "../../src/domain/dependency-graph"; @@ -9,7 +10,10 @@ import { exampleRegistryUrl } from "./data-registry"; import { PackumentNotFoundError } from "../../src/common-errors"; describe("dependency graph", () => { + // TODO: Maybe add some property-based tests + const somePackage = makeDomainName("com.some.package"); + const otherPackage = makeDomainName("com.other.package"); const someVersion = makeSemanticVersion("1.0.0"); describe("set node", () => { @@ -75,4 +79,29 @@ describe("dependency graph", () => { expect(actual).toBeTruthy(); }); }); + + describe("flatten", () => { + it("should flatten down correctly", () => { + const someNode = { + resolved: true, + source: exampleRegistryUrl, + dependencies: { [otherPackage]: someVersion }, + } as const; + const otherNode = { + resolved: false, + error: new PackumentNotFoundError(otherPackage), + } as const; + + let graph = emptyDependencyGraph; + graph = setGraphNode(graph, somePackage, someVersion, someNode); + graph = setGraphNode(graph, otherPackage, someVersion, otherNode); + + const actual = flattenDependencyGraph(graph); + + expect(actual).toEqual([ + [somePackage, someVersion, someNode], + [otherPackage, someVersion, otherNode], + ]); + }); + }); }); From 6a791e4093fdcdd907567ff80205fb38414e9070 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Mon, 8 Jul 2024 17:53:31 +0200 Subject: [PATCH 7/8] refactor: shorten --- src/cli/cmd-deps.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cli/cmd-deps.ts b/src/cli/cmd-deps.ts index 5e26b5ef..58fae434 100644 --- a/src/cli/cmd-deps.ts +++ b/src/cli/cmd-deps.ts @@ -101,7 +101,7 @@ export function makeDepsCmd( if (dependencyName === packageName) return; - log.notice("dependency", `${dependencyRef}`); + log.notice("dependency", dependencyRef); } ); From 99f89fbd5fd714705ec748255492a81e5c638972 Mon Sep 17 00:00:00 2001 From: Ramon Brullo Date: Mon, 8 Jul 2024 17:55:12 +0200 Subject: [PATCH 8/8] fix: package resolve bug --- src/services/dependency-resolving.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/dependency-resolving.ts b/src/services/dependency-resolving.ts index 0f2424a9..cb6f22fe 100644 --- a/src/services/dependency-resolving.ts +++ b/src/services/dependency-resolving.ts @@ -83,7 +83,7 @@ export function makeResolveDependency( if (!deep) return updatedGraph; return await resolveRecursively( - graph, + updatedGraph, sources, [...packagesToCheck.slice(1), ...recordEntries(dependencies)], deep