Fix: incorrect optional ignore in nested dependencies (#4448)

**Summary** Fixes #4445. The bug was introduced in https://github.com/yarnpkg/yarn/pull/3976/files#diff-7066979f95168f7bb59e4f9cfb3ba8fcR211 with an incomplete package optional check. This caused many transient dependencies to be ignored at linking stage when `--ignore-optional` flag is passed. This patch fixes the check to combine the flag with the package's actual optional mark. **Test plan** Manually verified #4445 is resolved. Also added a unit test.
yarnpkg · Sep 14, 2017 · ef8185b · ef8185b
1 parent cf3a5f2
commit ef8185b
Show file tree

Hide file tree

Showing 8 changed files with 34 additions and 11 deletions.
diff --git a/__tests__/commands/install/integration.js b/__tests__/commands/install/integration.js
@@ -64,6 +64,9 @@ test.concurrent('install optional subdependencies by default', async () => {
 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);
+    expect(await fs.exists(`${config.cwd}/node_modules/dep-c`)).toEqual(true);
+    expect(await fs.exists(`${config.cwd}/node_modules/dep-d`)).toEqual(true);
+    expect(await fs.exists(`${config.cwd}/node_modules/dep-e`)).toEqual(true);
   });
 });
 

diff --git a/__tests__/fixtures/install/install-optional-dependencies/dep-a/package.json b/__tests__/fixtures/install/install-optional-dependencies/dep-a/package.json
@@ -1,6 +1,9 @@
 {
     "name": "dep-a",
     "version": "1.0.0",
+    "dependencies": {
+        "dep-c": "file:../dep-c"
+    },
     "optionalDependencies": {
         "dep-b": "file:../dep-b"
     }

diff --git a/__tests__/fixtures/install/install-optional-dependencies/dep-c/package.json b/__tests__/fixtures/install/install-optional-dependencies/dep-c/package.json
@@ -0,0 +1,10 @@
+{
+    "name": "dep-c",
+    "version": "1.0.0",
+    "optionalDependencies": {
+        "dep-e": "file:../dep-e"
+    },
+    "dependencies": {
+        "dep-d": "file:../dep-d"
+    }
+}
diff --git a/__tests__/fixtures/install/install-optional-dependencies/dep-d/package.json b/__tests__/fixtures/install/install-optional-dependencies/dep-d/package.json
@@ -0,0 +1,7 @@
+{
+    "name": "dep-d",
+    "version": "1.0.0",
+    "dependencies": {
+        "dep-e": "file:../dep-e"
+    }
+}
diff --git a/__tests__/fixtures/install/install-optional-dependencies/dep-e/package.json b/__tests__/fixtures/install/install-optional-dependencies/dep-e/package.json
@@ -0,0 +1,4 @@
+{
+    "name": "dep-e",
+    "version": "1.0.0"
+}
diff --git a/src/package-hoister.js b/src/package-hoister.js
@@ -242,7 +242,7 @@ export default class PackageHoister {
           continue;
         }
 
-        const isMarkedAsOptional = !depinfo.pkg._reference || this.ignoreOptional;
+        const isMarkedAsOptional = depinfo.pkg._reference && depinfo.pkg._reference.optional && this.ignoreOptional;
         if (!depinfo.isRequired && !depinfo.isIncompatible && !isMarkedAsOptional) {
           depinfo.isRequired = true;
           depinfo.addHistory(`Mark as non-ignored because of usage by ${info.key}`);

diff --git a/src/package-request.js b/src/package-request.js
@@ -193,6 +193,7 @@ export default class PackageRequest {
     invariant(ref, 'Resolved package info has no package reference');
     ref.addRequest(this);
     ref.addPattern(this.pattern, resolved);
+    ref.addOptional(this.optional);
   }
 
   /**

diff --git a/src/package-resolver.js b/src/package-resolver.js
@@ -27,7 +27,7 @@ export type ResolverOptions = {|
 export default class PackageResolver {
   constructor(config: Config, lockfile: Lockfile, resolutionMap: ResolutionMap = new ResolutionMap(config)) {
     this.patternsByPackage = map();
-    this.fetchingPatterns = map();
+    this.fetchingPatterns = new Set();
     this.fetchingQueue = new BlockingQueue('resolver fetching');
     this.patterns = map();
     this.resolutionMap = resolutionMap;
@@ -59,9 +59,7 @@ export default class PackageResolver {
   };
 
   // patterns we've already resolved or are in the process of resolving
-  fetchingPatterns: {
-    [key: string]: true,
-  };
+  fetchingPatterns: Set<string>;
 
   // TODO
   fetchingQueue: BlockingQueue;
@@ -477,13 +475,11 @@ export default class PackageResolver {
       return;
     }
 
-    const fetchKey = `${req.registry}:${req.pattern}`;
-
-    if (this.fetchingPatterns[fetchKey]) {
+    const fetchKey = `${req.registry}:${req.pattern}:${String(req.optional)}`;
+    if (this.fetchingPatterns.has(fetchKey)) {
       return;
-    } else {
-      this.fetchingPatterns[fetchKey] = true;
     }
+    this.fetchingPatterns.add(fetchKey);
 
     if (this.activity) {
       this.activity.tick(req.pattern);
@@ -579,7 +575,6 @@ export default class PackageResolver {
       } else {
         this.resolutionMap.addToDelayQueue(req);
       }
-
       return null;
     }