From 831675966078b106ccbc0066ba41b5d21c458a16 Mon Sep 17 00:00:00 2001 From: Sebastian McKenzie Date: Wed, 15 Feb 2017 15:35:43 +0000 Subject: [PATCH] Prioritise popular transitive dependencies when hoisting (#2676) --- .../commands/install/integration-hoisting.js | 19 ++++ .../a/package.json | 7 ++ .../b-2/package.json | 4 + .../b/package.json | 8 ++ .../c/package.json | 7 ++ .../package.json | 5 + src/package-hoister.js | 91 +++++++++++++++++++ 7 files changed, 141 insertions(+) create mode 100644 __tests__/commands/install/integration-hoisting.js create mode 100644 __tests__/fixtures/install/install-should-prioritise-popular-transitive/a/package.json create mode 100644 __tests__/fixtures/install/install-should-prioritise-popular-transitive/b-2/package.json create mode 100644 __tests__/fixtures/install/install-should-prioritise-popular-transitive/b/package.json create mode 100644 __tests__/fixtures/install/install-should-prioritise-popular-transitive/c/package.json create mode 100644 __tests__/fixtures/install/install-should-prioritise-popular-transitive/package.json diff --git a/__tests__/commands/install/integration-hoisting.js b/__tests__/commands/install/integration-hoisting.js new file mode 100644 index 0000000000..ac0c63104b --- /dev/null +++ b/__tests__/commands/install/integration-hoisting.js @@ -0,0 +1,19 @@ +/* @flow */ + +import {getPackageVersion, runInstall} from '../_helpers.js'; +import * as fs from '../../../src/util/fs.js'; + +const assert = require('assert'); +const path = require('path'); + +jasmine.DEFAULT_TIMEOUT_INTERVAL = 120000; + +test.concurrent('install hoister should prioritise popular transitive dependencies', (): Promise => { + // a -> b -> b-2 + // -> c + // -> b-2 + return runInstall({}, 'install-should-prioritise-popular-transitive', async (config) => { + assert.equal(await getPackageVersion(config, 'b'), '2.0.0'); + assert.equal(await getPackageVersion(config, 'a/b'), '0.0.0'); + }); +}); diff --git a/__tests__/fixtures/install/install-should-prioritise-popular-transitive/a/package.json b/__tests__/fixtures/install/install-should-prioritise-popular-transitive/a/package.json new file mode 100644 index 0000000000..0f628dcd04 --- /dev/null +++ b/__tests__/fixtures/install/install-should-prioritise-popular-transitive/a/package.json @@ -0,0 +1,7 @@ +{ + "name": "a", + "version": "0.0.0", + "dependencies": { + "b": "file:../b" + } +} diff --git a/__tests__/fixtures/install/install-should-prioritise-popular-transitive/b-2/package.json b/__tests__/fixtures/install/install-should-prioritise-popular-transitive/b-2/package.json new file mode 100644 index 0000000000..57549744f3 --- /dev/null +++ b/__tests__/fixtures/install/install-should-prioritise-popular-transitive/b-2/package.json @@ -0,0 +1,4 @@ +{ + "name": "b", + "version": "2.0.0" +} diff --git a/__tests__/fixtures/install/install-should-prioritise-popular-transitive/b/package.json b/__tests__/fixtures/install/install-should-prioritise-popular-transitive/b/package.json new file mode 100644 index 0000000000..0d9daf714c --- /dev/null +++ b/__tests__/fixtures/install/install-should-prioritise-popular-transitive/b/package.json @@ -0,0 +1,8 @@ +{ + "name": "b", + "version": "0.0.0", + "dependencies": { + "b": "file:../b-2", + "c": "file:../c" + } +} diff --git a/__tests__/fixtures/install/install-should-prioritise-popular-transitive/c/package.json b/__tests__/fixtures/install/install-should-prioritise-popular-transitive/c/package.json new file mode 100644 index 0000000000..0c0656c6d6 --- /dev/null +++ b/__tests__/fixtures/install/install-should-prioritise-popular-transitive/c/package.json @@ -0,0 +1,7 @@ +{ + "name": "c", + "version": "0.0.0", + "dependencies": { + "b": "file:../b-2" + } +} diff --git a/__tests__/fixtures/install/install-should-prioritise-popular-transitive/package.json b/__tests__/fixtures/install/install-should-prioritise-popular-transitive/package.json new file mode 100644 index 0000000000..4e3bb4400b --- /dev/null +++ b/__tests__/fixtures/install/install-should-prioritise-popular-transitive/package.json @@ -0,0 +1,5 @@ +{ + "dependencies": { + "a": "file:a" + } +} diff --git a/src/package-hoister.js b/src/package-hoister.js index dbbe9cf58e..facc004879 100644 --- a/src/package-hoister.js +++ b/src/package-hoister.js @@ -87,6 +87,8 @@ export default class PackageHoister { */ seed(patterns: Array) { + this.prepass(patterns); + for (const pattern of this.resolver.dedupePatterns(patterns)) { this._seed(pattern); } @@ -409,6 +411,95 @@ export default class PackageHoister { info.addHistory(`New position = ${newKey}`); } + /** + * Perform a prepass and if there's multiple versions of the same package, hoist the one with + * the most dependents to the top. + */ + + prepass(patterns: Array) { + patterns = this.resolver.dedupePatterns(patterns).sort(); + + const occurences: { + [packageName: string]: { + [version: string]: { + pattern: string, + occurences: Set, + }, + }, + } = {}; + + // add an occuring package to the above data structure + const add = (pattern: string, ancestry: Array) => { + const pkg = this.resolver.getStrictResolvedPattern(pattern); + if (ancestry.indexOf(pkg) >= 0) { + // prevent recursive dependencies + return; + } + + const ref = pkg._reference; + invariant(ref, 'expected reference'); + + const versions = occurences[pkg.name] = occurences[pkg.name] || {}; + const version = versions[pkg.version] = versions[pkg.version] || {occurences: new Set(), pattern}; + version.occurences.add(ancestry[ancestry.length - 1]); + + for (const depPattern of ref.dependencies) { + add(depPattern, ancestry.concat(pkg)); + } + }; + + // get a list of root package names since we can't hoist other dependencies to these spots! + const rootPackageNames: Set = new Set(); + for (const pattern of patterns) { + const pkg = this.resolver.getStrictResolvedPattern(pattern); + rootPackageNames.add(pkg.name); + } + + // seed occurences + for (const pattern of patterns) { + add(pattern, []); + } + + for (const packageName of Object.keys(occurences).sort()) { + const versionOccurences = occurences[packageName]; + const versions = Object.keys(versionOccurences); + + if (versions.length === 1) { + // only one package type so we'll hoist this to the top anyway + continue; + } + + if (this.tree.get(packageName)) { + // a transitive dependency of a previously hoisted dependency exists + continue; + } + + if (rootPackageNames.has(packageName)) { + // can't replace top level packages + continue; + } + + let mostOccurenceCount; + let mostOccurencePattern; + for (const version of Object.keys(versionOccurences).sort()) { + const {occurences, pattern} = versionOccurences[version]; + const occurenceCount = occurences.size; + + if (!mostOccurenceCount || occurenceCount > mostOccurenceCount) { + mostOccurenceCount = occurenceCount; + mostOccurencePattern = pattern; + } + } + invariant(mostOccurencePattern, 'expected most occuring pattern'); + invariant(mostOccurenceCount, 'expected most occuring count'); + + // only hoist this module if it occured more than once + if (mostOccurenceCount > 1) { + this._seed(mostOccurencePattern); + } + } + } + /** * Produce a flattened list of module locations and manifests. */