From f80cbb580c733b76b2e3b273ab1930e121be07c2 Mon Sep 17 00:00:00 2001 From: Jeff Valore Date: Thu, 5 Apr 2018 05:42:45 -0400 Subject: [PATCH] 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 | 1 + __tests__/commands/upgrade.js | 33 +++++++++++++++++++ .../dev-dependency-no-warn/package.json | 6 ++++ .../upgrade/dev-dependency-no-warn/yarn.lock | 7 ++++ src/cli/commands/add.js | 8 +++-- 5 files changed, 53 insertions(+), 2 deletions(-) create mode 100644 __tests__/fixtures/upgrade/dev-dependency-no-warn/package.json create mode 100644 __tests__/fixtures/upgrade/dev-dependency-no-warn/yarn.lock diff --git a/__tests__/commands/upgrade-interactive.js b/__tests__/commands/upgrade-interactive.js index c1525333eb..e13c8b4dbb 100644 --- a/__tests__/commands/upgrade-interactive.js +++ b/__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 => { + config.commandName = 'upgrade-interactive'; return upgradeInteractive(config, reporter, flags, args); }); diff --git a/__tests__/commands/upgrade.js b/__tests__/commands/upgrade.js index 51f000ac67..6718a550a4 100644 --- a/__tests__/commands/upgrade.js +++ b/__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 => { + config.commandName = 'upgrade'; return upgrade(config, reporter, flags, args); }); @@ -281,6 +282,7 @@ test.concurrent('informs the type of dependency after upgrade', (): Promise => { + 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 => { + 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 => { + 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 => { + return buildRun( + reporters.BufferReporter, + fixturesLoc, + async (args, flags, config, reporter): Promise => { + 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 => { return runUpgrade(['left-pad@1.1.2'], {}, 'prune-offline-mirror', async (config): ?Promise => { 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 => { + config.commandName = 'upgrade'; + await upgrade(config, reporter, flags, args); const output = reporter.getBuffer(); diff --git a/__tests__/fixtures/upgrade/dev-dependency-no-warn/package.json b/__tests__/fixtures/upgrade/dev-dependency-no-warn/package.json new file mode 100644 index 0000000000..ec63e845c4 --- /dev/null +++ b/__tests__/fixtures/upgrade/dev-dependency-no-warn/package.json @@ -0,0 +1,6 @@ +{ + "private": "true", + "devDependencies": { + "left-pad": "^1.1.0" + } +} diff --git a/__tests__/fixtures/upgrade/dev-dependency-no-warn/yarn.lock b/__tests__/fixtures/upgrade/dev-dependency-no-warn/yarn.lock new file mode 100644 index 0000000000..00a5ec33b4 --- /dev/null +++ b/__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" diff --git a/src/cli/commands/add.js b/src/cli/commands/add.js index 617c676397..78631ba8fe 100644 --- a/src/cli/commands/add.js +++ b/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, 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)); } });