diff --git a/tools/yarn-cling/lib/hoisting.ts b/tools/yarn-cling/lib/hoisting.ts index 5f486c0828355..7031b9891108a 100644 --- a/tools/yarn-cling/lib/hoisting.ts +++ b/tools/yarn-cling/lib/hoisting.ts @@ -2,50 +2,148 @@ import { PackageLockPackage } from './types'; /** * Hoist package-lock dependencies in-place + * + * This happens in two phases: + * + * 1) Move every package into the parent scope (as long as it introduces no conflicts). + * This step mutates the dependency tree. + * 2) Once no more packages can be moved up, clean up the tree. This step mutates the + * tree declarations but cannot change versions of required packages. Two cleanups: + * 2a) Remove duplicates down the tree (same version that is inherited from above) + * 2b) Remove useless packages that aren't depended upon by anything in that subtree. + * To determine whether a package is useful or useless in a tree, we record + * each package's original dependencies before we start messing around in the + * tree. + * + * This two-phase process replaces a proces that did move-and-delete as one step, which + * sometimes would hoist a package into a place that was previously vacated by a conflicting + * version, thereby causing the wrong version to be loaded. + * + * Hoisting is still rather expensive on a large tree (~100ms), we should find ways to + * speed it up. */ -export function hoistDependencies(packageLockDeps: Record) { - let didChange; - do { - didChange = false; - simplify(packageLockDeps); - } while (didChange); +export function hoistDependencies(packageTree: PackageLockPackage) { + const originalDependencies = new Map(); + recordOriginalDependencies(packageTree); - // For each of the deps, move each dependency that has the same version into the current array - function simplify(dependencies: Record) { - for (const depPackage of Object.values(dependencies)) { - moveChildrenUp(depPackage, dependencies); + moveUp(packageTree); + removeDupes(packageTree, []); + removeUseless(packageTree); + + // Move the children of the parent onto the same level if there are no conflicts + function moveUp(node: PackageLockPackage, parent?: PackageLockPackage) { + if (!node.dependencies) { return; } + + // Recurse + for (const child of Object.values(node.dependencies)) { + moveUp(child, node); + } + + // Then push packages from the current node into its parent + if (parent) { + for (const [depName, depPackage] of Object.entries(node.dependencies)) { + if (!parent.dependencies?.[depName]) { + // It's new and there's no version conflict, we can move it up. + parent.dependencies![depName] = depPackage; + } + } } - return dependencies; } - // Move the children of the parent onto the same level if there are no conflicts - function moveChildrenUp(parent: PackageLockPackage, parentContainer: Record) { - if (!parent.dependencies) { return; } - - // Then push packages from the mutable parent into ITS parent - for (const [depName, depPackage] of Object.entries(parent.dependencies)) { - if (!parentContainer[depName]) { - // It's new, we can move it up. - parentContainer[depName] = depPackage; - delete parent.dependencies[depName]; - didChange = true; - - // Recurse on the package we just moved - moveChildrenUp(depPackage, parentContainer); - } else if (parentContainer[depName].version === depPackage.version) { - // Already exists, no conflict, delete the child, no need to recurse - delete parent.dependencies[depName]; - didChange = true; - } else { - // There is a conflict, leave the second package where it is, but do recurse. - moveChildrenUp(depPackage, parent.dependencies); + function removeDupes(node: PackageLockPackage, rootPath: Array) { + if (!node.dependencies) { return; } + + // Any dependencies here that are the same in the parent can be removed + for (const [depName, depPackage] of Object.entries(node.dependencies)) { + if (findInheritedDepVersion(depName, rootPath) === depPackage.version) { + delete node.dependencies[depName]; } } - // Cleanup for nice printing - if (Object.keys(parent.dependencies).length === 0) { - delete parent.dependencies; - didChange = true; + // Recurse + for (const child of Object.values(node.dependencies)) { + removeDupes(child, [node, ...rootPath]); } } + + function removeUseless(node: PackageLockPackage) { + if (!node.dependencies) { return; } + for (const [depName, depPkg] of Object.entries(node.dependencies)) { + if (!necessaryInTree(depName, depPkg.version, node)) { + delete node.dependencies[depName]; + } + } + + // Recurse + for (const child of Object.values(node.dependencies)) { + removeUseless(child); + } + + // If we ended up with empty dependencies, just get rid of the key (for clean printing) + if (Object.keys(node.dependencies).length === 0) { + delete node.dependencies; + } + } + + function findInheritedDepVersion(name: string, parentDependenciesChain: Array) { + for (const deps of parentDependenciesChain) { + if (deps.dependencies?.[name]) { + return deps.dependencies[name].version; + } + } + return undefined; + } + + function recordOriginalDependencies(node: PackageLockPackage) { + if (!node.dependencies) { return; } + + let list = originalDependencies.get(node); + if (!list) { + list = []; + originalDependencies.set(node, list); + } + + for (const [depName, depPkg] of Object.entries(node.dependencies)) { + list.push(`${depName}@${depPkg.version}`); + recordOriginalDependencies(depPkg); + } + } + + function necessaryInTree(name: string, version: string, tree: PackageLockPackage) { + if (originalDependencies.get(tree)?.includes(`${name}@${version}`)) { + return true; + } + if (!tree.dependencies) { return false; } + + for (const depPackage of Object.values(tree.dependencies)) { + if (necessaryInTree(name, version, depPackage)) { return true; } + } + return false; + } +} + +export function renderTree(tree: PackageLockPackage): string[] { + const ret = new Array(); + recurse(tree.dependencies ?? {}, []); + return ret.sort(compareSplit); + + function recurse(n: Record, parts: string[]) { + for (const [k, v] of Object.entries(n)) { + ret.push([...parts, k].join('.') + '=' + v.version); + recurse(v.dependencies ?? {}, [...parts, k]); + } + } + + function compareSplit(a: string, b: string): number { + // Sort so that: 'a=1', 'a.b=2' get sorted in that order. + const as = a.split(/\.|=/g); + const bs = b.split(/\.|=/g); + + for (let i = 0; i < as.length && i < bs.length; i++) { + const cmp = as[i].localeCompare(bs[i]); + if (cmp !== 0) { return cmp; } + } + + return as.length - bs.length; + } } diff --git a/tools/yarn-cling/lib/index.ts b/tools/yarn-cling/lib/index.ts index 75a3605166255..0ca255547e5e2 100644 --- a/tools/yarn-cling/lib/index.ts +++ b/tools/yarn-cling/lib/index.ts @@ -38,7 +38,7 @@ export async function generateShrinkwrap(options: ShrinkwrapOptions): Promise; +type DependencyTree = PackageLockPackage; test('nonconflicting tree gets flattened', () => { // GIVEN - const tree: Tree = { + const tree: DependencyTree = pkg('*', { stringutil: { version: '1.0.0', dependencies: { - leftpad: { version: '2.0.0' } + leftpad: { version: '2.0.0' }, }, }, numutil: { version: '3.0.0', dependencies: { - isodd: { version: '4.0.0' } + isodd: { version: '4.0.0' }, }, }, - }; + }); // WHEN hoistDependencies(tree); // THEN - expect(tree).toEqual({ - stringutil: { version: '1.0.0' }, - leftpad: { version: '2.0.0' }, - numutil: { version: '3.0.0' }, - isodd: { version: '4.0.0' }, - }); + expect(renderTree(tree)).toEqual([ + 'isodd=4.0.0', + 'leftpad=2.0.0', + 'numutil=3.0.0', + 'stringutil=1.0.0', + ]); }); test('matching versions get deduped', () => { // GIVEN - const tree: Tree = { + const tree: DependencyTree = pkg('*', { stringutil: { version: '1.0.0', dependencies: { - leftpad: { version: '2.0.0' } + leftpad: { version: '2.0.0' }, }, }, numutil: { version: '3.0.0', dependencies: { leftpad: { version: '2.0.0' }, - isodd: { version: '4.0.0' } + isodd: { version: '4.0.0' }, }, }, - }; + }); // WHEN hoistDependencies(tree); // THEN - expect(tree).toEqual({ - stringutil: { version: '1.0.0' }, - leftpad: { version: '2.0.0' }, - numutil: { version: '3.0.0' }, - isodd: { version: '4.0.0' }, - }); + expect(renderTree(tree)).toEqual([ + 'isodd=4.0.0', + 'leftpad=2.0.0', + 'numutil=3.0.0', + 'stringutil=1.0.0', + ]); }); test('conflicting versions get left in place', () => { // GIVEN - const tree: Tree = { + const tree: DependencyTree = pkg('*', { stringutil: { version: '1.0.0', dependencies: { - leftpad: { version: '2.0.0' } + leftpad: { version: '2.0.0' }, }, }, numutil: { version: '3.0.0', dependencies: { leftpad: { version: '5.0.0' }, - isodd: { version: '4.0.0' } + isodd: { version: '4.0.0' }, }, }, - }; + }); // WHEN hoistDependencies(tree); // THEN - expect(tree).toEqual({ - stringutil: { version: '1.0.0' }, - leftpad: { version: '2.0.0' }, - numutil: { - version: '3.0.0', - dependencies: { - leftpad: { version: '5.0.0' }, - }, - }, - isodd: { version: '4.0.0' }, - }); + expect(renderTree(tree)).toEqual([ + 'isodd=4.0.0', + 'leftpad=2.0.0', + 'numutil=3.0.0', + 'numutil.leftpad=5.0.0', + 'stringutil=1.0.0', + ]); }); test('dependencies of deduped packages are not hoisted into useless positions', () => { // GIVEN + const tree: DependencyTree = pkg('*', { + stringutil: pkg('1.0.0', { + leftpad: pkg('2.0.0', { + spacemaker: pkg('3.0.0'), + }), + }), + leftpad: pkg('2.0.0', { + spacemaker: pkg('3.0.0'), + }), + spacemaker: pkg('4.0.0'), + }); - const tree: Tree = { - stringutil: { - version: '1.0.0', - dependencies: { - leftpad: { - version: '2.0.0', - dependencies: { - spacemaker: { version: '3.0.0' }, - } - } - }, - }, - leftpad: { - version: '2.0.0', - dependencies: { - spacemaker: { version: '3.0.0' }, - } - }, - spacemaker: { version: '4.0.0' } - }; + // WHEN + hoistDependencies(tree); + + // THEN + expect(renderTree(tree)).toEqual([ + 'leftpad=2.0.0', + 'leftpad.spacemaker=3.0.0', + 'spacemaker=4.0.0', + 'stringutil=1.0.0', + ]); +}); + +test('dont hoist into a parent if it would cause an incorrect version there', () => { + // GIVEN + const tree: DependencyTree = pkg('*', { + stringutil: pkg('1.0.0', { + spacemaker: pkg('10.0.0'), + leftPad: pkg('2.0.0', { + spacemaker: pkg('3.0.0'), + }), + }), + leftPad: pkg('1.0.0'), // Prevents previous leftPad from being hoisted + }); // WHEN hoistDependencies(tree); // THEN - expect(tree).toEqual({ - stringutil: { version: '1.0.0' }, - leftpad: { - version: '2.0.0', - dependencies: { - spacemaker: { version: '3.0.0' } - } - }, - spacemaker: { version: '4.0.0' }, + expect(renderTree(tree)).toEqual([ + 'leftPad=1.0.0', + 'spacemaker=10.0.0', + 'stringutil=1.0.0', + 'stringutil.leftPad=2.0.0', + 'stringutil.leftPad.spacemaker=3.0.0', + ]); +}); + +test('order of hoisting shouldnt produce a broken situation', () => { + // GIVEN + const tree: DependencyTree = pkg('*', { + stringutil: pkg('1.0.0', { + wrapper: pkg('100.0.0', { + leftPad: pkg('2.0.0', { + spacemaker: pkg('3.0.0'), + }), + }), + spacemaker: pkg('4.0.0'), // Prevents spacemaker from being hoisted here, but then leftPad also shouldn't be + }), }); -}); \ No newline at end of file + + // WHEN + hoistDependencies(tree); + + // THEN + /* // Both answers are fine but the current algorithm picks the 2nd + expect(renderTree(tree)).toEqual([ + 'leftPad=2.0.0', + 'spacemaker=3.0.0', + 'stringutil=1.0.0', + 'stringutil.spacemaker=4.0.0', + 'wrapper=100.0.0', + ]); + */ + expect(renderTree(tree)).toEqual([ + 'leftPad=2.0.0', + 'leftPad.spacemaker=3.0.0', + 'spacemaker=4.0.0', + 'stringutil=1.0.0', + 'wrapper=100.0.0', + ]); +}); + +function pkg(version: string, dependencies?: Record): PackageLockPackage { + return { + version, + ...dependencies? { dependencies } : undefined, + }; +} +