Fixed #1242 - missing transitive dependencies (#2279)

* Fixed #1242 - missing transitive dependencies

* nits and fixes

* added a few more tests that cover current behavior

* redisabled test

* typo fix

* typo fix

* fixes for @cpojer

* comments

__tests__/commands/install/integration.js

@@ -719,20 +719,6 @@ test('install a scoped module from authed private registry with a missing traili
   });
 });
 
-test.concurrent('install a module with incompatible optional dependency should skip dependency',
-  (): Promise<void> => {
-    return runInstall({}, 'install-should-skip-incompatible-optional-dep', async (config) => {
-      assert.ok(!(await fs.exists(path.join(config.cwd, 'node_modules', 'dep-incompatible'))));
-    });
-  });
-
-test.concurrent('install a module with incompatible optional dependency should skip transient dependencies',
-  (): Promise<void> => {
-    return runInstall({}, 'install-should-skip-incompatible-optional-dep', async (config) => {
-      assert.ok(!(await fs.exists(path.join(config.cwd, 'node_modules', 'dep-a'))));
-    });
-  });
-
 test.concurrent('install will not overwrite files in symlinked scoped directories', async (): Promise<void> => {
   await runInstall({}, 'install-dont-overwrite-linked-scoped', async (config): Promise<void> => {
     const dependencyPath = path.join(config.cwd, 'node_modules', '@fakescope', 'fake-dependency');
@@ -750,13 +736,45 @@ test.concurrent('install will not overwrite files in symlinked scoped directorie
   });
 });
 
-test.concurrent.skip('install incompatible optional dependency should still install shared child dependencies',
+test.concurrent('install a module with incompatible optional dependency should skip dependency',
+  (): Promise<void> => {
+    return runInstall({}, 'install-should-skip-incompatible-optional-dep', async (config) => {
+      assert.ok(!(await fs.exists(path.join(config.cwd, 'node_modules', 'dep-incompatible'))));
+    });
+  });
+
+test.concurrent('install a module with incompatible optional dependency should skip transient dependencies',
+  (): Promise<void> => {
+    return runInstall({}, 'install-should-skip-incompatible-optional-dep', async (config) => {
+      assert.ok(!(await fs.exists(path.join(config.cwd, 'node_modules', 'dep-a'))));
+    });
+  });
+
+// this tests for a problem occuring due to optional dependency incompatible with os, in this case fsevents
+// this would fail on os's incompatible with fsevents, which is everything except osx.
+if (process.platform !== 'darwin') {
+  test.concurrent('install incompatible optional dependency should still install shared child dependencies',
+    (): Promise<void> => {
+      return runInstall({}, 'install-should-not-skip-required-shared-deps', async (config) => {
+        assert.ok(await fs.exists(path.join(config.cwd, 'node_modules', 'deep-extend')));
+        assert.ok(await fs.exists(path.join(config.cwd, 'node_modules', 'ini')));
+        assert.ok(await fs.exists(path.join(config.cwd, 'node_modules', 'strip-json-comments')));
+      });
+    });
+}
+
+// Covers current behavior, issue opened whether this should be changed https://github.com/yarnpkg/yarn/issues/2274
+test.concurrent('optional dependency that fails to build should still be installed',
+  (): Promise<void> => {
+    return runInstall({}, 'should-install-failing-optional-deps', async (config) => {
+      assert.ok(await fs.exists(path.join(config.cwd, 'node_modules', 'optional-failing')));
+    });
+  });
+
+test.concurrent('a subdependency of an optional dependency that fails should be installed',
   (): Promise<void> => {
-    // this tests for a problem occuring due to optional dependency incompatible with os, in this case fsevents
-    // this would fail on os's incompatible with fsevents, which is everything except osx.
-    return runInstall({}, 'install-should-not-skip-required-shared-deps', async (config) => {
-      assert.ok(await fs.exists(path.join(config.cwd, 'node_modules', 'deep-extend')));
-      assert.ok(await fs.exists(path.join(config.cwd, 'node_modules', 'ini')));
-      assert.ok(await fs.exists(path.join(config.cwd, 'node_modules', 'strip-json-comments')));
+    return runInstall({}, 'should-install-failing-optional-sub-deps', async (config) => {
+      assert.ok(await fs.exists(path.join(config.cwd, 'node_modules', 'optional-failing')));
+      assert.ok(await fs.exists(path.join(config.cwd, 'node_modules', 'sub-dep')));
     });
   });

__tests__/fixtures/install/should-install-failing-optional-deps/optional-failing/install.js

@@ -0,0 +1 @@
+process.exit(1);

__tests__/fixtures/install/should-install-failing-optional-deps/optional-failing/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "optional-failing",
+  "version": "0.0.0",
+  "scripts": {
+    "install": "node install.js"
+  }
+}

__tests__/fixtures/install/should-install-failing-optional-deps/package.json

@@ -0,0 +1,5 @@
+{
+  "optionalDependencies": {
+    "optional-failing": "file:optional-failing"
+  }
+}

__tests__/fixtures/install/should-install-failing-optional-sub-deps/optional-failing/install.js

@@ -0,0 +1 @@
+process.exit(1);

__tests__/fixtures/install/should-install-failing-optional-sub-deps/optional-failing/package.json

@@ -0,0 +1,10 @@
+{
+  "name": "optional-failing",
+  "version": "0.0.0",
+  "scripts": {
+    "install": "node install.js"
+  },
+  "dependencies": {
+    "sub-dep": "file:../sub-dep"
+  }
+}

__tests__/fixtures/install/should-install-failing-optional-sub-deps/package.json

@@ -0,0 +1,5 @@
+{
+  "optionalDependencies": {
+    "optional-failing": "file:optional-failing"
+  }
+}

__tests__/fixtures/install/should-install-failing-optional-sub-deps/sub-dep/install.js

@@ -0,0 +1 @@
+process.exit(0);

__tests__/fixtures/install/should-install-failing-optional-sub-deps/sub-dep/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "sub-dep",
+  "version": "0.0.0",
+  "scripts": {
+    "install": "node install.js"
+  }
+}

__tests__/package-hoister.js

@@ -28,7 +28,7 @@ test('Produces valid destination paths for scoped modules', () => {
     _reference: (({}: any): PackageReference),
   }: any): Manifest);
 
-  const info = new HoistManifest(key, parts, pkg, '', false);
+  const info = new HoistManifest(key, parts, pkg, '', () => false);
 
   const tree = new Map([
     ['@scoped/dep', info],

src/package-hoister.js

@@ -13,8 +13,8 @@ type Parts = Array<string>;
 let historyCounter = 0;
 
 export class HoistManifest {
-  constructor(key: string, parts: Parts, pkg: Manifest, loc: string, isIgnored: boolean) {
-    this.ignore = isIgnored;
+  constructor(key: string, parts: Parts, pkg: Manifest, loc: string, isIgnored: () => boolean) {
+    this.isIgnored = isIgnored;
     this.loc = loc;
     this.pkg = pkg;
 
@@ -27,7 +27,7 @@ export class HoistManifest {
     this.addHistory(`Start position = ${key}`);
   }
 
-  ignore: boolean;
+  isIgnored: () => boolean;
   pkg: Manifest;
   loc: string;
   parts: Parts;
@@ -130,13 +130,17 @@ export default class PackageHoister {
 
     //
     let parentParts: Parts = [];
-    let isIgnored = ref.ignore;
+    let isIgnored = () => ref.ignore;
 
     if (parent) {
       if (!this.tree.get(parent.key)) {
         return null;
       }
-      isIgnored = isIgnored || parent.ignore;
+      // 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 && parent.isIgnored();
+      }
       parentParts = parent.parts;
     }
 
@@ -181,9 +185,9 @@ export default class PackageHoister {
       const existing = this.tree.get(checkKey);
       if (existing) {
         if (existing.loc === info.loc) {
-          // deduping an unignored reference to an ignored one
-          if (existing.ignore && !info.ignore) {
-            existing.ignore = false;
+          // switch to non ignored if earlier deduped version was ignored
+          if (existing.isIgnored() && !info.isIgnored()) {
+            existing.isIgnored = info.isIgnored;
           }
 
           existing.addHistory(`Deduped ${fullKey} to this item`);
@@ -277,7 +281,6 @@ export default class PackageHoister {
     // remove this item from the `tree` map so we can ignore it
     this.tree.delete(key);
 
-    //
     const {parts, duplicate} = this.getNewParts(key, info, rawParts.slice());
     const newKey = this.implodeKey(parts);
     const oldKey = key;
@@ -389,7 +392,7 @@ export default class PackageHoister {
       const ref = info.pkg._reference;
       invariant(ref, 'expected reference');
 
-      if (info.ignore) {
+      if (info.isIgnored()) {
         info.addHistory('Deleted as this module was ignored');
       } else {
         visibleFlatTree.push([loc, info]);