Merge pull request microsoft#878 from elliottsj/rush-install-optional

pgonzal · web-flow · commit a16659d01606 · 2018-10-16T16:26:43.000-07:00
Install optional dependencies, except when using npm&lt;5.0.0
diff --git a/apps/rush-lib/src/logic/InstallManager.ts b/apps/rush-lib/src/logic/InstallManager.ts
@@ -781,23 +781,6 @@ export class InstallManager {
           }
 
           // Run "npm install" in the common folder
-
-          // NOTE:
-          // we do NOT install optional dependencies for Rush, as it seems that optional dependencies do not
-          // work properly with shrinkwrap. Consider the "fsevents" package. This is a Mac specific package
-          // which is an optional second-order dependency. Optional dependencies work by attempting to install
-          // the package, but removes the package if the install failed.
-          // This means that someone running generate on a Mac WILL have fsevents included in their shrinkwrap.
-          // When someone using Windows attempts to install from the shrinkwrap, the install will fail.
-          //
-          // If someone generates the shrinkwrap using Windows, then fsevents will NOT be listed in the shrinkwrap.
-          // When someone using Mac attempts to install from the shrinkwrap, (as of NPM 4), they will NOT have the
-          // optional dependency installed.
-          //
-          // One possible solution would be to have the shrinkwrap include information about whether the dependency
-          // is optional or not, but it does not appear to do so. Also, this would result in strange behavior where
-          // people would have different node_modules based on their system.
-
           const installArgs: string[] = [ 'install' ];
           this._pushConfigurationArgs(installArgs, options);
 
@@ -975,15 +958,34 @@ export class InstallManager {
    */
   private _pushConfigurationArgs(args: string[], options: IInstallManagerOptions): void {
     if (this._rushConfiguration.packageManager === 'npm') {
-      args.push('--no-optional');
+      if (semver.lt(this._rushConfiguration.packageManagerToolVersion, '5.0.0')) {
+        // NOTE:
+        //
+        // When using an npm version older than v5.0.0, we do NOT install optional dependencies for
+        // Rush, because npm does not generate the shrinkwrap file consistently across platforms.
+        //
+        // Consider the "fsevents" package. This is a Mac specific package
+        // which is an optional second-order dependency. Optional dependencies work by attempting to install
+        // the package, but removes the package if the install failed.
+        // This means that someone running generate on a Mac WILL have fsevents included in their shrinkwrap.
+        // When someone using Windows attempts to install from the shrinkwrap, the install will fail.
+        //
+        // If someone generates the shrinkwrap using Windows, then fsevents will NOT be listed in the shrinkwrap.
+        // When someone using Mac attempts to install from the shrinkwrap, they will NOT have the
+        // optional dependency installed.
+        //
+        // This issue has been fixed as of npm v5.0.0: https://github.com/npm/npm/releases/tag/v5.0.0
+        //
+        // For more context, see https://github.com/Microsoft/web-build-tools/issues/761#issuecomment-428689600
+        args.push('--no-optional');
+      }
       args.push('--cache', this._rushConfiguration.npmCacheFolder);
       args.push('--tmp', this._rushConfiguration.npmTmpFolder);
 
       if (options.collectLogFile) {
         args.push('--verbose');
       }
     } else if (this._rushConfiguration.packageManager === 'pnpm') {
-      args.push('--no-optional');
       args.push('--store', this._rushConfiguration.pnpmStoreFolder);
 
       // we are using the --no-lock flag for now, which unfortunately prints a warning, but should be OK
@@ -1009,7 +1011,6 @@ export class InstallManager {
         args.push('--strict-peer-dependencies');
       }
     } else if (this._rushConfiguration.packageManager === 'yarn') {
-      args.push('--ignore-optional');
       args.push('--link-folder', 'yarn-link');
       args.push('--cache-folder', this._rushConfiguration.yarnCacheFolder);
 
diff --git a/apps/rush-lib/src/logic/npm/NpmLinkManager.ts b/apps/rush-lib/src/logic/npm/NpmLinkManager.ts
@@ -296,7 +296,7 @@ export class NpmLinkManager extends BaseLinkManager {
             throw Error(`The dependency "${dependency.name}" needed by "${localPackage.name}"`
               + ` was not found in the common folder -- do you need to run "rush install"?`);
           } else {
-            console.log(colors.yellow('Skipping optional dependency: ' + dependency.name));
+            console.log('Skipping optional dependency: ' + dependency.name);
           }
         }
       }
diff --git a/common/changes/@microsoft/rush/rush-install-optional_2018-10-16-03-26.json b/common/changes/@microsoft/rush/rush-install-optional_2018-10-16-03-26.json
@@ -0,0 +1,11 @@
+{
+  "changes": [
+    {
+      "comment": "Install optional dependencies, except w/ npm<5.0.0",
+      "packageName": "@microsoft/rush",
+      "type": "none"
+    }
+  ],
+  "packageName": "@microsoft/rush",
+  "email": "elliottsj@users.noreply.github.com"
+}

Original file line number	Diff line number	Diff line change
`@@ -296,7 +296,7 @@ export class NpmLinkManager extends BaseLinkManager {`
`296`	`296`	throw Error(`The dependency "${dependency.name}" needed by "${localPackage.name}"`
`297`	`297`	+ ` was not found in the common folder -- do you need to run "rush install"?`);
`298`	`298`	`} else {`
`299`		`- console.log(colors.yellow('Skipping optional dependency: ' + dependency.name));`
	`299`	`+ console.log('Skipping optional dependency: ' + dependency.name);`
`300`	`300`	`}`
`301`	`301`	`}`
`302`	`302`	`}`