Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prioritise popular transitive dependencies when hoisting #2676

Merged
merged 2 commits into from
Feb 15, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions __tests__/commands/install/integration-hoisting.js
Original file line number Diff line number Diff line change
@@ -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<void> => {
// 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');
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "a",
"version": "0.0.0",
"dependencies": {
"b": "file:../b"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "b",
"version": "2.0.0"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"name": "b",
"version": "0.0.0",
"dependencies": {
"b": "file:../b-2",
"c": "file:../c"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "c",
"version": "0.0.0",
"dependencies": {
"b": "file:../b-2"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"dependencies": {
"a": "file:a"
}
}
91 changes: 91 additions & 0 deletions src/package-hoister.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ export default class PackageHoister {
*/

seed(patterns: Array<string>) {
this.prepass(patterns);

for (const pattern of this.resolver.dedupePatterns(patterns)) {
this._seed(pattern);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to seed the patterns again here?

}
Expand Down Expand Up @@ -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<string>) {
patterns = this.resolver.dedupePatterns(patterns).sort();

const occurences: {
[packageName: string]: {
[version: string]: {
pattern: string,
occurences: Set<Manifest>,
},
},
} = {};

// add an occuring package to the above data structure
const add = (pattern: string, ancestry: Array<Manifest>) => {
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<string> = 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.
*/
Expand Down