fix(upgrade): No longer warn when upgrading a devDependency (#5606)

* fix(upgrade): No longer warn when upgrading a devDependency

fixes #4840

**Summary**

Previously the upgrade command would call add with the list of packages
to upgrade to. No "--dev"
flag would be carried over and this list would contain all dependencies.
As a result, Add would
report a warning `{package} is already in "devDependencies". Please
remove existing entry first
before adding it to "dependencies".`

I am using the existing `config.commandName` to decide whether or not to
display the warning.

I also updated the jest snapshots, because I was tired of the warning
about 40-some obsolete snapshots, so the snapshot changes here aren't
relevant to the functionality, just cleaning up.

**Test plan**

Added regression test in `__tests__/commands/upgrade.js`.

Upgrade tests now set `config.commandName` which is an existing
property, but is not normally set by tests. I wanted to set it for all
tests in `_helpers.js` but we don't actually specify the command we are
going to test when we build a test runner.

* refactor code based on PR feedback

__tests__/commands/upgrade-interactive.js

@@ -11,6 +11,7 @@ const path = require('path');
 
 const fixturesLoc = path.join(__dirname, '..', 'fixtures', 'upgrade-interactive');
 const runUpgrade = buildRun.bind(null, ConsoleReporter, fixturesLoc, (args, flags, config, reporter): Promise<void> => {
+  config.commandName = 'upgrade-interactive';
   return upgradeInteractive(config, reporter, flags, args);
 });
 

__tests__/commands/upgrade.js

@@ -12,6 +12,7 @@ const path = require('path');
 
 const fixturesLoc = path.join(__dirname, '..', 'fixtures', 'upgrade');
 const runUpgrade = buildRun.bind(null, ConsoleReporter, fixturesLoc, (args, flags, config, reporter): Promise<void> => {
+  config.commandName = 'upgrade';
   return upgrade(config, reporter, flags, args);
 });
 
@@ -281,6 +282,7 @@ test.concurrent('informs the type of dependency after upgrade', (): Promise<void
     reporters.BufferReporter,
     fixturesLoc,
     async (args, flags, config, reporter): Promise<void> => {
+      config.commandName = 'upgrade';
       await upgrade(config, reporter, flags, args);
 
       const output = reporter.getBuffer();
@@ -309,6 +311,8 @@ test.concurrent('warns when peer dependency is not met after upgrade', (): Promi
     reporters.BufferReporter,
     fixturesLoc,
     async (args, flags, config, reporter): Promise<void> => {
+      config.commandName = 'upgrade';
+
       await upgrade(config, reporter, flags, args);
 
       const output = reporter.getBuffer();
@@ -331,6 +335,8 @@ test.concurrent("doesn't warn when peer dependency is still met after upgrade",
     reporters.BufferReporter,
     fixturesLoc,
     async (args, flags, config, reporter): Promise<void> => {
+      config.commandName = 'upgrade';
+
       await upgrade(config, reporter, flags, args);
 
       const output = reporter.getBuffer();
@@ -348,6 +354,31 @@ test.concurrent("doesn't warn when peer dependency is still met after upgrade",
   );
 });
 
+// Regression test for #4840
+test.concurrent("doesn't warn when upgrading a devDependency", (): Promise<void> => {
+  return buildRun(
+    reporters.BufferReporter,
+    fixturesLoc,
+    async (args, flags, config, reporter): Promise<void> => {
+      config.commandName = 'upgrade';
+
+      await upgrade(config, reporter, flags, args);
+
+      const output = reporter.getBuffer();
+      const warnings = output.filter(entry => entry.type === 'warning');
+
+      expect(
+        warnings.some(warning => {
+          return warning.data.toString().toLowerCase().indexOf('is already in') > -1;
+        }),
+      ).toEqual(false);
+    },
+    ['left-pad'],
+    {},
+    'dev-dependency-no-warn',
+  );
+});
+
 test.concurrent('can prune the offline mirror', (): Promise<void> => {
   return runUpgrade(['[email protected]'], {}, 'prune-offline-mirror', async (config): ?Promise<void> => {
     await expectInstalledDependency(config, 'left-pad', '1.1.2', '1.1.2');
@@ -386,6 +417,8 @@ test.concurrent('--latest works if there is an install script on a hoisted depen
     reporters.BufferReporter,
     fixturesLoc,
     async (args, flags, config, reporter): Promise<void> => {
+      config.commandName = 'upgrade';
+
       await upgrade(config, reporter, flags, args);
 
       const output = reporter.getBuffer();

__tests__/fixtures/upgrade/dev-dependency-no-warn/package.json

@@ -0,0 +1,6 @@
+{
+  "private": "true",
+  "devDependencies": {
+    "left-pad": "^1.1.0"
+  }
+}

__tests__/fixtures/upgrade/dev-dependency-no-warn/yarn.lock

@@ -0,0 +1,7 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
+# yarn lockfile v1
+
+
+left-pad@^1.1.0:
+  version "1.1.1"
+  resolved "https://registry.yarnpkg.com/left-pad/-/left-pad-1.1.1.tgz#ca566bbdd84b90cc5969ac1726fda51f9d936a3c"

src/cli/commands/add.js

@@ -19,6 +19,8 @@ import invariant from 'invariant';
 import path from 'path';
 import semver from 'semver';
 
+const SILENCE_DEPENDENCY_TYPE_WARNINGS = ['upgrade', 'upgrade-interactive'];
+
 export class Add extends Install {
   constructor(args: Array<string>, flags: Object, config: Config, reporter: Reporter, lockfile: Lockfile) {
     const workspaceRootIsCwd = config.cwd === config.lockfileFolder;
@@ -244,8 +246,10 @@ export class Add extends Install {
 
       object[dependencyType] = object[dependencyType] || {};
       object[dependencyType][pkgName] = version;
-
-      if (dependencyType !== this.flagToOrigin) {
+      if (
+        SILENCE_DEPENDENCY_TYPE_WARNINGS.indexOf(this.config.commandName) === -1 &&
+        dependencyType !== this.flagToOrigin
+      ) {
         this.reporter.warn(this.reporter.lang('moduleAlreadyInManifest', pkgName, dependencyType, this.flagToOrigin));
       }
     });