Fix: Prevent optional subdependencies from being installed with --ign…

…ore-optional (#3976)

**Summary**

Fixes #2666.

**Test plan**

Additional unit tests.

__tests__/commands/install/integration.js

@@ -48,6 +48,18 @@ async function mockConstants(base: Config, mocks: Object, cb: (config: Config) =
 beforeEach(request.__resetAuthedRequests);
 afterEach(request.__resetAuthedRequests);
 
+test.concurrent('install optional subdependencies by default', async () => {
+  await runInstall({}, 'install-optional-dependencies', async (config): Promise<void> => {
+    expect(await fs.exists(`${config.cwd}/node_modules/dep-b`)).toEqual(true);
+  });
+});
+
+test.concurrent('installing with --ignore-optional should not install optional subdependencies', async () => {
+  await runInstall({ignoreOptional: true}, 'install-optional-dependencies', async (config): Promise<void> => {
+    expect(await fs.exists(`${config.cwd}/node_modules/dep-b`)).toEqual(false);
+  });
+});
+
 test.concurrent('packages installed through the link protocol should validate all peer dependencies', async () => {
   await runInstall({checkFiles: true}, 'check-files-should-not-cross-symlinks', async (config): Promise<void> => {
     expect(

__tests__/fixtures/install/install-optional-dependencies/dep-a/package.json

@@ -0,0 +1,7 @@
+{
+    "name": "dep-a",
+    "version": "1.0.0",
+    "optionalDependencies": {
+        "dep-b": "file:../dep-b"
+    }
+}

__tests__/fixtures/install/install-optional-dependencies/dep-b/package.json

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

__tests__/fixtures/install/install-optional-dependencies/package.json

@@ -0,0 +1,5 @@
+{
+    "dependencies": {
+        "dep-a": "file:./dep-a"
+    }
+}

src/cli/commands/install.js

@@ -278,12 +278,7 @@ export class Install {
 
       pushDeps('dependencies', projectManifestJson, {hint: null, optional: false}, true);
       pushDeps('devDependencies', projectManifestJson, {hint: 'dev', optional: false}, !this.config.production);
-      pushDeps(
-        'optionalDependencies',
-        projectManifestJson,
-        {hint: 'optional', optional: true},
-        !this.flags.ignoreOptional,
-      );
+      pushDeps('optionalDependencies', projectManifestJson, {hint: 'optional', optional: true}, true);
 
       if (this.config.workspacesEnabled) {
         const workspaces = await this.config.resolveWorkspaces(path.dirname(loc), projectManifestJson);
@@ -461,7 +456,10 @@ export class Install {
       // remove integrity hash to make this operation atomic
       await this.integrityChecker.removeIntegrityFile();
       this.reporter.step(curr, total, this.reporter.lang('linkingDependencies'), emoji.get('link'));
-      await this.linker.init(flattenedTopLevelPatterns, this.flags.linkDuplicates, workspaceLayout);
+      await this.linker.init(flattenedTopLevelPatterns, workspaceLayout, {
+        linkDuplicates: this.flags.linkDuplicates,
+        ignoreOptional: this.flags.ignoreOptional,
+      });
     });
 
     steps.push(async (curr: number, total: number) => {

src/package-hoister.js

@@ -54,10 +54,12 @@ export class HoistManifest {
 }
 
 export default class PackageHoister {
-  constructor(config: Config, resolver: PackageResolver) {
+  constructor(config: Config, resolver: PackageResolver, {ignoreOptional}: {ignoreOptional: ?boolean} = {}) {
     this.resolver = resolver;
     this.config = config;
 
+    this.ignoreOptional = ignoreOptional;
+
     this.taintedKeys = new Map();
     this.levelQueue = [];
     this.tree = new Map();
@@ -66,6 +68,8 @@ export default class PackageHoister {
   resolver: PackageResolver;
   config: Config;
 
+  ignoreOptional: ?boolean;
+
   levelQueue: Array<[string, HoistManifest]>;
   tree: Map<string, HoistManifest>;
   taintedKeys: Map<string, HoistManifest>;
@@ -141,16 +145,19 @@ export default class PackageHoister {
 
     //
     let parentParts: Parts = [];
+
     const isIncompatible = ref.incompatible;
-    let isRequired = isDirectRequire && !ref.ignore && !isIncompatible;
+    const isMarkedAsOptional = ref.optional && this.ignoreOptional;
+
+    let isRequired = isDirectRequire && !ref.ignore && !isIncompatible && !isMarkedAsOptional;
 
     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 (!isDirectRequire && !isIncompatible && parent.isRequired) {
+      if (!isDirectRequire && !isIncompatible && parent.isRequired && !isMarkedAsOptional) {
         isRequired = true;
       }
       parentParts = parent.parts;
@@ -196,7 +203,13 @@ export default class PackageHoister {
 
       for (const depPattern of ref.dependencies) {
         const depinfo = this._lookupDependency(info, depPattern);
-        if (depinfo && !depinfo.isRequired && !depinfo.isIncompatible) {
+
+        if (!depinfo) {
+          continue;
+        }
+
+        const isMarkedAsOptional = !depinfo.pkg._reference || this.ignoreOptional;
+        if (!depinfo.isRequired && !depinfo.isIncompatible && !isMarkedAsOptional) {
           depinfo.isRequired = true;
           depinfo.addHistory(`Mark as non-ignored because of usage by ${info.key}`);
           toVisit.push(depinfo);
@@ -544,7 +557,6 @@ export default class PackageHoister {
     for (const [loc, info] of flatTree) {
       const ref = info.pkg._reference;
       invariant(ref, 'expected reference');
-
       if (!info.isRequired) {
         info.addHistory('Deleted as this module was ignored');
       } else {

src/package-linker.js

@@ -119,18 +119,21 @@ export default class PackageLinker {
     }
   }
 
-  getFlatHoistedTree(patterns: Array<string>): Promise<HoistManifestTuples> {
-    const hoister = new PackageHoister(this.config, this.resolver);
+  getFlatHoistedTree(
+    patterns: Array<string>,
+    {ignoreOptional}: {ignoreOptional: ?boolean} = {},
+  ): Promise<HoistManifestTuples> {
+    const hoister = new PackageHoister(this.config, this.resolver, {ignoreOptional});
     hoister.seed(patterns);
     return Promise.resolve(hoister.init());
   }
 
   async copyModules(
     patterns: Array<string>,
-    linkDuplicates: boolean,
     workspaceLayout?: WorkspaceLayout,
+    {linkDuplicates, ignoreOptional}: {linkDuplicates: ?boolean, ignoreOptional: ?boolean} = {},
   ): Promise<void> {
-    let flatTree = await this.getFlatHoistedTree(patterns);
+    let flatTree = await this.getFlatHoistedTree(patterns, {ignoreOptional});
 
     // sorted tree makes file creation and copying not to interfere with each other
     flatTree = flatTree.sort(function(dep1, dep2): number {
@@ -429,8 +432,12 @@ export default class PackageLinker {
     return range === '*' || satisfiesWithPreleases(version, range, this.config.looseSemver);
   }
 
-  async init(patterns: Array<string>, linkDuplicates: boolean, workspaceLayout?: WorkspaceLayout): Promise<void> {
+  async init(
+    patterns: Array<string>,
+    workspaceLayout?: WorkspaceLayout,
+    {linkDuplicates, ignoreOptional}: {linkDuplicates: ?boolean, ignoreOptional: ?boolean} = {},
+  ): Promise<void> {
     this.resolvePeerModules();
-    await this.copyModules(patterns, linkDuplicates, workspaceLayout);
+    await this.copyModules(patterns, workspaceLayout, {linkDuplicates, ignoreOptional});
   }
 }