fix(linking): Handle missing package when using --ignore-optional (#5059

)

**Summary**

Closes #5054, closes #4876, closes #5152.

Currently when running `yarn --ignore-optional` required dependencies can be marked as optional because they exist in the tree of `optionalDependencies` of one of the dependencies (but it's required by some other non-optional dependency).

For example: `once` depends on `wrappy`, but `wrappy` is also in the chain from `fsevents` (`glob` -> `inflight` -> `wrappy`) which is an optional dependency of `chokidar`. So if the first `wrappy` comes from an "optional chain" when yarn processes it, it ends up being marked as `_reference.optional = true`, and the package hoister therefore doesn't mark it as required.

When running the released version of yarn with https://github.com/spalger/reproduce-issues/blob/master/yarn-ignores-non-optional-dependencies you'll see

```
~/dev/reproduce-issues/yarn-ignores-non-optional-dependencies
$ yarn --ignore-optional
yarn install v1.3.2-20171204.1856
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 📃  Building fresh packages...
✨  Done in 0.17s.

~/dev/reproduce-issues/yarn-ignores-non-optional-dependencies
$ yarn check --verify-tree
yarn check v1.3.2-20171204.1856
error "once#wrappy" not installed
error An unexpected error occurred: "Found 1 errors.".
```

This fix checks whether or not the parent is marked as required, and if so, marking the dependency as required _unless_ it's listed in the parent's `optionalDependencies`.

**Test plan**

I tested this implementation with both https://github.com/spalger/reproduce-issues/blob/master/yarn-ignores-non-optional-dependencies and #4876, and in both cases `yarn check --verify-tree` succeeded after running `yarn --ignore-optional`.

Also added automated tests.

__tests__/commands/install/integration.js

@@ -811,17 +811,24 @@ test.concurrent('a subdependency of an optional dependency that fails should be
   });
 });
 
-test.concurrent('a sub-dependency should be non-optional if any parents mark it non-optional', async (): Promise<
-  void,
-> => {
-  let thrown = false;
-  try {
-    await runInstall({}, 'failing-sub-dep-optional-and-normal', () => {});
-  } catch (err) {
-    thrown = true;
-    expect(err.message).toContain('sub-failing: Command failed');
-  }
-  expect(thrown).toEqual(true);
+test.concurrent('a sub-dependency should be non-optional if any parents mark it non-optional', (): Promise<void> => {
+  return runInstall(
+    {ignoreOptional: true},
+    'install-sub-dependency-if-any-parents-mark-it-non-optional',
+    async config => {
+      const deps = await fs.readdir(path.join(config.cwd, 'node_modules'));
+
+      expect(deps).toEqual([
+        '.yarn-integrity',
+        'normal-dep',
+        'normal-sub-dep',
+        'normal-sub-sub-dep',
+        'sub-dep',
+        'sub-dep-2',
+        'sub-sub-dep',
+      ]);
+    },
+  );
 });
 
 test.concurrent('should not loose dependencies when installing with --production', (): Promise<void> => {

__tests__/fixtures/install/install-sub-dependency-if-any-parents-mark-it-non-optional/normal-dep/package.json

@@ -0,0 +1,11 @@
+{
+  "name": "normal-dep",
+  "version": "0.0.0",
+  "dependencies": {
+    "sub-dep": "file:../sub-dep",
+    "normal-sub-dep": "file:../normal-sub-dep"
+  },
+  "optionalDependencies": {
+    "optional-sub-dep": "file:../optional-sub-dep"
+  }
+}

__tests__/fixtures/install/install-sub-dependency-if-any-parents-mark-it-non-optional/normal-sub-dep/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "normal-sub-dep",
+  "version": "0.0.0",
+  "dependencies": {
+    "normal-sub-sub-dep": "file:../normal-sub-sub-dep"
+  }
+}

__tests__/fixtures/install/install-sub-dependency-if-any-parents-mark-it-non-optional/normal-sub-sub-dep/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "normal-sub-sub-dep",
+  "version": "0.0.0",
+  "dependencies": {
+    "sub-dep-2": "file:../sub-dep-2"
+  }
+}

__tests__/fixtures/install/install-sub-dependency-if-any-parents-mark-it-non-optional/optional-dep/package.json

@@ -0,0 +1,8 @@
+{
+  "name": "optional-dep",
+  "version": "0.0.0",
+  "dependencies": {
+    "sub-dep": "file:../sub-dep",
+    "sub-dep-2": "file:../sub-dep-2"
+  }
+}

__tests__/fixtures/install/install-sub-dependency-if-any-parents-mark-it-non-optional/optional-sub-dep/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "optional-sub-dep",
+  "version": "0.0.0",
+  "dependencies": {
+    "normal-sub-sub-dep": "file:../normal-sub-sub-dep"
+  }
+}

__tests__/fixtures/install/install-sub-dependency-if-any-parents-mark-it-non-optional/sub-dep-2/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "sub-dep-2",
+  "version": "0.0.0",
+  "dependencies": {
+    "sub-sub-dep": "file:../sub-sub-dep"
+  }
+}

__tests__/fixtures/install/install-sub-dependency-if-any-parents-mark-it-non-optional/sub-dep/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "sub-dep",
+  "version": "0.0.0"
+}

__tests__/fixtures/install/install-sub-dependency-if-any-parents-mark-it-non-optional/sub-sub-dep/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "sub-sub-dep",
+  "version": "0.0.0"
+}

src/cli/commands/install.js

@@ -282,7 +282,12 @@ export class Install {
         }
       }
 
-      const pushDeps = (depType, manifest: Object, {hint, optional}, isUsed) => {
+      const pushDeps = (
+        depType,
+        manifest: Object,
+        {hint, optional}: {hint: ?constants.RequestHint, optional: boolean},
+        isUsed,
+      ) => {
         if (ignoreUnusedPatterns && !isUsed) {
           return;
         }

src/constants.js

@@ -124,3 +124,5 @@ export const VERSION_COLOR_SCHEME: {[key: string]: VersionColor} = {
 };
 
 export type VersionColor = 'red' | 'yellow' | 'green' | 'white';
+
+export type RequestHint = 'dev' | 'optional' | 'resolution' | 'workspaces';

src/package-hoister.js

@@ -262,7 +262,14 @@ export default class PackageHoister {
           continue;
         }
 
-        const isMarkedAsOptional = depinfo.pkg._reference && depinfo.pkg._reference.optional && this.ignoreOptional;
+        const depRef = depinfo.pkg._reference;
+
+        // If it's marked as optional, but the parent is required and the
+        // dependency was not listed in `optionalDependencies`, then we mark the
+        // dependency as required.
+        const isMarkedAsOptional =
+          depRef && depRef.optional && this.ignoreOptional && !(info.isRequired && depRef.hint !== 'optional');
+
         if (!depinfo.isRequired && !depinfo.isIncompatible && !isMarkedAsOptional) {
           depinfo.isRequired = true;
           depinfo.addHistory(`Mark as non-ignored because of usage by ${info.key}`);

src/package-reference.js

@@ -7,13 +7,15 @@ import type PackageRequest from './package-request.js';
 import type PackageResolver from './package-resolver.js';
 import type {RegistryNames} from './registries/index.js';
 import {entries} from './util/misc.js';
+import type {RequestHint} from './constants';
 
 export default class PackageReference {
   constructor(request: PackageRequest, info: Manifest, remote: PackageRemote) {
     this.resolver = request.resolver;
     this.lockfile = request.lockfile;
     this.requests = [];
     this.config = request.config;
+    this.hint = request.hint;
 
     this.registry = remote.registry;
     this.version = info.version;
@@ -44,6 +46,7 @@ export default class PackageReference {
   version: string;
   uid: string;
   optional: ?boolean;
+  hint: ?RequestHint;
   ignore: boolean;
   incompatible: boolean;
   fresh: boolean;

src/package-request.js

@@ -37,6 +37,7 @@ export default class PackageRequest {
     this.reporter = resolver.reporter;
     this.resolver = resolver;
     this.optional = req.optional;
+    this.hint = req.hint;
     this.pattern = req.pattern;
     this.config = resolver.config;
     this.foundInfo = null;
@@ -55,6 +56,7 @@ export default class PackageRequest {
   config: Config;
   registry: ResolverRegistryNames;
   optional: boolean;
+  hint: ?constants.RequestHint;
   foundInfo: ?Manifest;
 
   getLocked(remoteType: FetcherNames): ?Manifest {
@@ -288,6 +290,7 @@ export default class PackageRequest {
       deps.push(depPattern);
       promises.push(
         this.resolver.find({
+          hint: 'optional',
           pattern: depPattern,
           registry: remote.registry,
           optional: true,
@@ -303,6 +306,7 @@ export default class PackageRequest {
         deps.push(depPattern);
         promises.push(
           this.resolver.find({
+            hint: 'dev',
             pattern: depPattern,
             registry: remote.registry,
             optional: false,

src/types.js

@@ -6,6 +6,7 @@ import type PackageRequest from './package-request.js';
 import type {FetcherNames} from './fetchers/index.js';
 import type {Reporter} from './reporters/index.js';
 import type Config from './config.js';
+import type {RequestHint} from './constants';
 
 export type CLIFunction = (config: Config, reporter: Reporter, flags: Object, args: Array<string>) => CLIFunctionReturn;
 
@@ -18,7 +19,7 @@ export type DependencyRequestPattern = {
   pattern: string,
   registry: RegistryNames,
   optional: boolean,
-  hint?: ?string,
+  hint?: ?RequestHint,
   parentNames?: Array<string>,
   parentRequest?: ?PackageRequest,
   workspaceName?: string,