diff --git a/__tests__/commands/install/integration.js b/__tests__/commands/install/integration.js index bc0547f8d0..4930c6fa91 100644 --- a/__tests__/commands/install/integration.js +++ b/__tests__/commands/install/integration.js @@ -157,6 +157,25 @@ test.concurrent("production mode with deduped dev dep shouldn't be removed", asy }); }); +test.concurrent("production mode dep on package in dev deps shouldn't be removed", async () => { + await runInstall({production: true}, 'install-prod-deduped-direct-dev-dep', async (config) => { + assert.equal( + (await fs.readJson(path.join(config.cwd, 'node_modules', 'a', 'package.json'))).version, + '1.0.0', + ); + + assert.equal( + (await fs.readJson(path.join(config.cwd, 'node_modules', 'b', 'package.json'))).version, + '1.0.0', + ); + + assert.equal( + (await fs.readJson(path.join(config.cwd, 'node_modules', 'c', 'package.json'))).version, + '1.0.0', + ); + }); +}); + test.concurrent('hoisting should factor ignored dependencies', async () => { // you should only modify this test if you know what you're doing // when we calculate hoisting we need to factor in ignored dependencies in it diff --git a/__tests__/fixtures/install/install-prod-deduped-direct-dev-dep/a/package.json b/__tests__/fixtures/install/install-prod-deduped-direct-dev-dep/a/package.json new file mode 100644 index 0000000000..be5b70f271 --- /dev/null +++ b/__tests__/fixtures/install/install-prod-deduped-direct-dev-dep/a/package.json @@ -0,0 +1,7 @@ +{ + "name": "a", + "version": "1.0.0", + "dependencies": { + "b": "file:../b" + } +} diff --git a/__tests__/fixtures/install/install-prod-deduped-direct-dev-dep/b/c/package.json b/__tests__/fixtures/install/install-prod-deduped-direct-dev-dep/b/c/package.json new file mode 100644 index 0000000000..abd3384930 --- /dev/null +++ b/__tests__/fixtures/install/install-prod-deduped-direct-dev-dep/b/c/package.json @@ -0,0 +1,4 @@ +{ + "name": "c", + "version": "1.0.0" +} diff --git a/__tests__/fixtures/install/install-prod-deduped-direct-dev-dep/b/package.json b/__tests__/fixtures/install/install-prod-deduped-direct-dev-dep/b/package.json new file mode 100644 index 0000000000..018eedcd05 --- /dev/null +++ b/__tests__/fixtures/install/install-prod-deduped-direct-dev-dep/b/package.json @@ -0,0 +1,7 @@ +{ + "name": "b", + "version": "1.0.0", + "dependencies": { + "c": "file:c" + } +} diff --git a/__tests__/fixtures/install/install-prod-deduped-direct-dev-dep/package.json b/__tests__/fixtures/install/install-prod-deduped-direct-dev-dep/package.json new file mode 100644 index 0000000000..b08417d92b --- /dev/null +++ b/__tests__/fixtures/install/install-prod-deduped-direct-dev-dep/package.json @@ -0,0 +1,8 @@ +{ + "dependencies": { + "a": "file:a" + }, + "devDependencies": { + "b": "file:b" + } +} diff --git a/__tests__/package-hoister.js b/__tests__/package-hoister.js index a1698b8aeb..d36819743f 100644 --- a/__tests__/package-hoister.js +++ b/__tests__/package-hoister.js @@ -110,7 +110,7 @@ test('Produces valid destination paths for scoped modules', () => { _reference: (({}: any): PackageReference), }: any): Manifest); - const info = new HoistManifest(key, parts, pkg, '', false, false); + const info = new HoistManifest(key, parts, pkg, '', true, false); const tree = new Map([ ['@scoped/dep', info], diff --git a/src/cli/commands/install.js b/src/cli/commands/install.js index c3fd07ad9e..cdbcba2184 100644 --- a/src/cli/commands/install.js +++ b/src/cli/commands/install.js @@ -251,6 +251,7 @@ export class Install { pattern += '@' + depMap[name]; } + // normalization made sure packages are mentioned only once if (isUsed) { usedPatterns.push(pattern); } else { @@ -352,12 +353,9 @@ export class Install { const ref = manifest._reference; invariant(ref, 'expected package reference'); - if (ref.requests.length === 1) { - // this module was only depended on once by the root so we can safely ignore it - // if it was requested more than once then ignoring it would break a transitive - // dep that resolved to it - ref.ignore = true; - } + // just mark the package as ignored. if the package is used by a required package, the hoister + // will take care of that. + ref.ignore = true; } } diff --git a/src/package-compatibility.js b/src/package-compatibility.js index fd6ab8d6b9..39acff4e49 100644 --- a/src/package-compatibility.js +++ b/src/package-compatibility.js @@ -126,6 +126,7 @@ export default class PackageCompatibility { if (ref.optional) { ref.ignore = true; + ref.incompatible = true; reporter.warn(`${human}: ${msg}`); if (!didIgnore) { diff --git a/src/package-hoister.js b/src/package-hoister.js index facc004879..5234557ab8 100644 --- a/src/package-hoister.js +++ b/src/package-hoister.js @@ -13,9 +13,9 @@ type Parts = Array; let historyCounter = 0; export class HoistManifest { - constructor(key: string, parts: Parts, pkg: Manifest, loc: string, isIgnored: boolean, inheritIsIgnored: boolean) { - this.isIgnored = isIgnored; - this.inheritIsIgnored = inheritIsIgnored; + constructor(key: string, parts: Parts, pkg: Manifest, loc: string, isRequired: boolean, isIncompatible: boolean) { + this.isRequired = isRequired; + this.isIncompatible = isIncompatible; this.loc = loc; this.pkg = pkg; @@ -28,8 +28,8 @@ export class HoistManifest { this.addHistory(`Start position = ${key}`); } - isIgnored: boolean; - inheritIsIgnored: boolean; + isRequired: boolean; + isIncompatible: boolean; pkg: Manifest; loc: string; parts: Parts; @@ -96,7 +96,7 @@ export default class PackageHoister { while (true) { let queue = this.levelQueue; if (!queue.length) { - this._propagateNonIgnored(); + this._propagateRequired(); return; } @@ -135,8 +135,8 @@ export default class PackageHoister { // let parentParts: Parts = []; - let isIgnored = ref.ignore; - let inheritIsIgnored = false; + const isIncompatible = ref.incompatible; + let isRequired = !parent && !ref.ignore && !isIncompatible; if (parent) { if (!this.tree.get(parent.key)) { @@ -144,9 +144,8 @@ export default class PackageHoister { } // non ignored dependencies inherit parent's ignored status // parent may transition from ignored to non ignored when hoisted if it is used in another non ignored branch - if (!isIgnored && parent.isIgnored) { - isIgnored = parent.isIgnored; - inheritIsIgnored = true; + if (!isRequired && !isIncompatible && parent.isRequired) { + isRequired = true; } parentParts = parent.parts; } @@ -155,7 +154,7 @@ export default class PackageHoister { const loc: string = this.config.generateHardModulePath(ref); const parts = parentParts.concat(pkg.name); const key: string = this.implodeKey(parts); - const info: HoistManifest = new HoistManifest(key, parts, pkg, loc, isIgnored, inheritIsIgnored); + const info: HoistManifest = new HoistManifest(key, parts, pkg, loc, isRequired, isIncompatible); // this.tree.set(key, info); @@ -173,13 +172,13 @@ export default class PackageHoister { * Propagate inherited ignore statuses from non-ignored to ignored packages */ - _propagateNonIgnored() { + _propagateRequired() { // const toVisit: Array = []; // enumerate all non-ignored packages for (const entry of this.tree.entries()) { - if (!entry[1].isIgnored) { + if (entry[1].isRequired) { toVisit.push(entry[1]); } } @@ -192,9 +191,9 @@ export default class PackageHoister { for (const depPattern of ref.dependencies) { const depinfo = this._lookupDependency(info, depPattern); - if (depinfo && depinfo.isIgnored && depinfo.inheritIsIgnored) { - depinfo.isIgnored = false; - info.addHistory(`Mark as non-ignored because of usage by ${info.key}`); + if (depinfo && !depinfo.isRequired && !depinfo.isIncompatible) { + depinfo.isRequired = true; + depinfo.addHistory(`Mark as non-ignored because of usage by ${info.key}`); toVisit.push(depinfo); } } @@ -247,12 +246,14 @@ export default class PackageHoister { const existing = this.tree.get(checkKey); if (existing) { if (existing.loc === info.loc) { - // switch to non ignored if earlier deduped version was ignored - if (existing.isIgnored && !info.isIgnored) { - existing.isIgnored = info.isIgnored; + // switch to non ignored if earlier deduped version was ignored (must be compatible) + if (!existing.isRequired && info.isRequired) { + existing.addHistory(`Deduped ${fullKey} to this item, marking as required`); + existing.isRequired = true; + } else { + existing.addHistory(`Deduped ${fullKey} to this item`); } - existing.addHistory(`Deduped ${fullKey} to this item`); return {parts: checkParts, duplicate: true}; } else { // everything above will be shadowed and this is a conflict @@ -543,7 +544,7 @@ export default class PackageHoister { const ref = info.pkg._reference; invariant(ref, 'expected reference'); - if (info.isIgnored) { + if (!info.isRequired) { info.addHistory('Deleted as this module was ignored'); } else { visibleFlatTree.push([loc, info]); diff --git a/src/package-install-scripts.js b/src/package-install-scripts.js index df96359833..81241a9973 100644 --- a/src/package-install-scripts.js +++ b/src/package-install-scripts.js @@ -110,6 +110,7 @@ export default class PackageInstallScripts { if (ref.optional) { ref.ignore = true; + ref.incompatible = true; this.reporter.warn(this.reporter.lang('optionalModuleScriptFail', err.message)); this.reporter.info(this.reporter.lang('optionalModuleFail')); diff --git a/src/package-reference.js b/src/package-reference.js index 7f72e37c71..6ba2433ddd 100644 --- a/src/package-reference.js +++ b/src/package-reference.js @@ -33,6 +33,7 @@ export default class PackageReference { this.optional = null; this.root = false; this.ignore = false; + this.incompatible = false; this.fresh = false; this.location = null; this.addRequest(request); @@ -48,6 +49,7 @@ export default class PackageReference { uid: string; optional: ?boolean; ignore: boolean; + incompatible: boolean; fresh: boolean; dependencies: Array; patterns: Array;