Fix missing subdeps in production install when those are present in d…

…evDependencies list (#2921) (#3065)

* Test for install skipping subdependencies when those are named in root devDependencies

* Add 'incompatible' flag to references, use that to ignore incompatible packages instead of faulty inherit logic. Fixes #2819

* Unconditionally mark packages as ignored, hoister now fully corrects transitive uses

__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

__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"
+  }
+}

__tests__/fixtures/install/install-prod-deduped-direct-dev-dep/b/c/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "c",
+  "version": "1.0.0"
+}

__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"
+  }
+}

__tests__/fixtures/install/install-prod-deduped-direct-dev-dep/package.json

@@ -0,0 +1,8 @@
+{
+  "dependencies": {
+    "a": "file:a"
+  },
+  "devDependencies": {
+    "b": "file: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],

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;
     }
   }
 

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) {

src/package-hoister.js

@@ -13,9 +13,9 @@ type Parts = Array<string>;
 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,18 +135,17 @@ 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)) {
         return null;
       }
       // 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<HoistManifest> = [];
 
     // 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]);

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'));
 

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<string>;
   patterns: Array<string>;