Skip to content

Commit 63330ad

Browse files
authored
chore(yarn-cling): breaks on dependency cycle (#18188)
In a recent upgrade, one of our dependencies (`json-diff`) has taken a dependency on a cluster of packages that have dependency cycles between them. Specifically: ``` json-diff => cli-color => [ d => es5-ext => es6-iterator => d ]. json-diff => cli-color => d => [ es5-ext => es6-iterator => es5-ext ]. json-diff => cli-color => [ d => es5-ext => es6-iterator => es6-symbol => d ]. json-diff => cli-color => [ d => es5-ext => es6-symbol => d ]. json-diff => cli-color => [ es5-ext => es6-iterator => d => es5-ext ]. json-diff => cli-color => [ es5-ext => es6-iterator => es6-symbol => d => es5-ext ]. json-diff => cli-color => [ es5-ext => es6-symbol => d => es5-ext ]. json-diff => cli-color => [ es6-iterator => d => es5-ext => es6-iterator ]. json-diff => cli-color => [ es6-iterator => es5-ext => es6-iterator ]. json-diff => cli-color => [ es6-iterator => es6-symbol => d => es5-ext => es6-iterator ]. json-diff => cli-color => es6-iterator => [ es6-symbol => d => es5-ext => es6-symbol ]. json-diff => cli-color => memoizee => es6-weak-map => [ es6-symbol => d => es5-ext => es6-iterator => es6-symbol ]. ``` `yarn-cling` used to go into infinite recursion trying to resolve this dependency tree, as it was not prepared to handle cycles. Add a dependency breaker. Since I wasn't sure whether or not this might break the dependency tree, add a validation step as well. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent be7acfd commit 63330ad

File tree

1 file changed

+63
-4
lines changed

1 file changed

+63
-4
lines changed

tools/@aws-cdk/yarn-cling/lib/index.ts

Lines changed: 63 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@ export async function generateShrinkwrap(options: ShrinkwrapOptions): Promise<Pa
4141
hoistDependencies({ version: '*', dependencies: lock.dependencies });
4242
}
4343

44+
validateTree(lock);
45+
4446
if (options.outputFile) {
4547
// Write the shrinkwrap file
4648
await fs.writeFile(options.outputFile, JSON.stringify(lock, undefined, 2), { encoding: 'utf8' });
@@ -55,22 +57,37 @@ async function generateLockFile(pkgJson: PackageJson, yarnLock: YarnLock, rootDi
5557
version: pkgJson.version,
5658
lockfileVersion: 1,
5759
requires: true,
58-
dependencies: await dependenciesFor(pkgJson.dependencies || {}, yarnLock, rootDir),
60+
dependencies: await dependenciesFor(pkgJson.dependencies || {}, yarnLock, rootDir, [pkgJson.name]),
5961
};
6062

6163
checkRequiredVersions(lockFile);
6264

6365
return lockFile;
6466
}
6567

68+
const CYCLES_REPORTED = new Set<string>();
69+
6670
// eslint-disable-next-line max-len
67-
async function dependenciesFor(deps: Record<string, string>, yarnLock: YarnLock, rootDir: string): Promise<Record<string, PackageLockPackage>> {
71+
async function dependenciesFor(deps: Record<string, string>, yarnLock: YarnLock, rootDir: string, dependencyPath: string[]): Promise<Record<string, PackageLockPackage>> {
6872
const ret: Record<string, PackageLockPackage> = {};
6973

7074
// Get rid of any monorepo symlinks
7175
rootDir = await fs.realpath(rootDir);
7276

7377
for (const [depName, versionRange] of Object.entries(deps)) {
78+
if (dependencyPath.includes(depName)) {
79+
const index = dependencyPath.indexOf(depName);
80+
const beforeCycle = dependencyPath.slice(0, index);
81+
const inCycle = [...dependencyPath.slice(index), depName];
82+
const cycleString = inCycle.join(' => ');
83+
if (!CYCLES_REPORTED.has(cycleString)) {
84+
// eslint-disable-next-line no-console
85+
console.warn(`Dependency cycle: ${beforeCycle.join(' => ')} => [ ${cycleString} ]. Dropping dependency '${inCycle.slice(-2).join(' => ')}'.`);
86+
CYCLES_REPORTED.add(cycleString);
87+
}
88+
continue;
89+
}
90+
7491
const depDir = await findPackageDir(depName, rootDir);
7592
const depPkgJsonFile = path.join(depDir, 'package.json');
7693
const depPkgJson = await loadPackageJson(depPkgJsonFile);
@@ -89,14 +106,14 @@ async function dependenciesFor(deps: Record<string, string>, yarnLock: YarnLock,
89106
integrity: yarnResolved.integrity,
90107
resolved: yarnResolved.resolved,
91108
requires: depPkgJson.dependencies,
92-
dependencies: await dependenciesFor(depPkgJson.dependencies || {}, yarnLock, depDir),
109+
dependencies: await dependenciesFor(depPkgJson.dependencies || {}, yarnLock, depDir, [...dependencyPath, depName]),
93110
};
94111
} else {
95112
// Comes from monorepo, just use whatever's in package.json
96113
ret[depName] = {
97114
version: depPkgJson.version,
98115
requires: depPkgJson.dependencies,
99-
dependencies: await dependenciesFor(depPkgJson.dependencies || {}, yarnLock, depDir),
116+
dependencies: await dependenciesFor(depPkgJson.dependencies || {}, yarnLock, depDir, [...dependencyPath, depName]),
100117
};
101118
}
102119

@@ -264,4 +281,46 @@ export function checkRequiredVersions(root: PackageLock | PackageLockPackage) {
264281
}
265282
return undefined;
266283
}
284+
}
285+
286+
/**
287+
* Check that all packages still resolve their dependencies to the right versions
288+
*
289+
* We have manipulated the tree a bunch. Do a sanity check to ensure that all declared
290+
* dependencies are satisfied.
291+
*/
292+
function validateTree(lock: PackageLock) {
293+
let failed = false;
294+
recurse(lock, [lock]);
295+
if (failed) {
296+
throw new Error('Could not satisfy one or more dependencies');
297+
}
298+
299+
function recurse(pkg: PackageLockEntry, rootPath: PackageLockEntry[]) {
300+
for (const pack of Object.values(pkg.dependencies ?? {})) {
301+
const p = [pack, ...rootPath];
302+
checkRequiresOf(pack, p);
303+
recurse(pack, p);
304+
}
305+
}
306+
307+
// rootPath: most specific one first
308+
function checkRequiresOf(pack: PackageLockPackage, rootPath: PackageLockEntry[]) {
309+
for (const [name, declaredRange] of Object.entries(pack.requires ?? {})) {
310+
const foundVersion = rootPath.map((p) => p.dependencies?.[name]?.version).find(isDefined);
311+
if (!foundVersion) {
312+
// eslint-disable-next-line no-console
313+
console.error(`Dependency on ${name} not satisfied: not found`);
314+
failed = true;
315+
} else if (!semver.satisfies(foundVersion, declaredRange)) {
316+
// eslint-disable-next-line no-console
317+
console.error(`Dependency on ${name} not satisfied: declared range '${declaredRange}', found '${foundVersion}'`);
318+
failed = true;
319+
}
320+
}
321+
}
322+
}
323+
324+
function isDefined<A>(x: A): x is NonNullable<A> {
325+
return x !== undefined;
267326
}

0 commit comments

Comments
 (0)