Upgrade transient deps during upgrades. (yarnpkg#4636)

* [yarnpkg#4476] Upgrade transient deps during upgrades.

* Rename 'transient' to 'transitive'

* dont upgrade direct deps unless requested, add verbose upgrade logging

* upgrade-interactive reuse lockfile cleaning from upgrade.js

__tests__/commands/upgrade.js

@@ -31,6 +31,11 @@ const expectInstalledDevDependency = async (config, name, range, expectedVersion
   await _expectDependency('devDependencies', config, name, range, expectedVersion);
 };
 
+const expectInstalledTransitiveDependency = async (config, name, range, expectedVersion) => {
+  const lockfile = explodeLockfile(await fs.readFile(path.join(config.cwd, 'yarn.lock')));
+  expect(lockfile).toContainPackage(`${name}@${range}:`, expectedVersion);
+};
+
 expect.extend({
   toContainPackage(lockfile, ...args): Object {
     const [pattern, expectedVersion] = args;
@@ -68,6 +73,32 @@ test.concurrent('works with no arguments', (): Promise<void> => {
   });
 });
 
+test.concurrent('upgrades transitive deps when no arguments', (): Promise<void> => {
+  return runUpgrade([], {}, 'with-subdep', async (config): ?Promise<void> => {
+    await expectInstalledDependency(config, 'strip-ansi', '^2.0.1', '2.0.1');
+    await expectInstalledTransitiveDependency(config, 'ansi-regex', '^1.0.0', '1.1.1');
+    await expectInstalledDependency(config, 'array-union', '^1.0.1', '1.0.2');
+    await expectInstalledTransitiveDependency(config, 'array-uniq', '^1.0.1', '1.0.3');
+  });
+});
+
+test.concurrent('does not upgrade transitive deps that are also a direct dependency', (): Promise<void> => {
+  return runUpgrade(['strip-ansi'], {}, 'with-subdep-also-direct', async (config): ?Promise<void> => {
+    await expectInstalledDependency(config, 'strip-ansi', '^2.0.1', '2.0.1');
+    await expectInstalledTransitiveDependency(config, 'ansi-regex', '^1.0.0', '1.0.0');
+    await expectInstalledDependency(config, 'ansi-regex', '^1.0.0', '1.0.0');
+  });
+});
+
+test.concurrent('does not upgrade transitive deps when specific package upgraded', (): Promise<void> => {
+  return runUpgrade(['strip-ansi'], {}, 'with-subdep', async (config): ?Promise<void> => {
+    await expectInstalledDependency(config, 'strip-ansi', '^2.0.1', '2.0.1');
+    await expectInstalledTransitiveDependency(config, 'ansi-regex', '^1.0.0', '1.1.1');
+    await expectInstalledDependency(config, 'array-union', '^1.0.1', '1.0.1');
+    await expectInstalledTransitiveDependency(config, 'array-uniq', '^1.0.1', '1.0.1');
+  });
+});
+
 test.concurrent('works with single argument', (): Promise<void> => {
   return runUpgrade(['max-safe-integer'], {}, 'single-package', async (config): ?Promise<void> => {
     await expectInstalledDependency(config, 'left-pad', '^1.0.0', '1.0.0');

__tests__/fixtures/upgrade/with-subdep-also-direct/package.json

@@ -0,0 +1,6 @@
+{
+  "dependencies": {
+    "strip-ansi": "^2.0.1",
+    "ansi-regex": "^1.0.0"
+  }
+}

__tests__/fixtures/upgrade/with-subdep-also-direct/yarn.lock

@@ -0,0 +1,10 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
+# yarn lockfile v1
+ansi-regex@^1.0.0:
+  version "1.0.0"
+  resolved "https://registry.npmjs.org/ansi-regex/-/ansi-regex-1.0.0.tgz#54c7ce13af71e436348666484c44516ab9bc144e"
+strip-ansi@^2.0.1:
+  version "2.0.1"
+  resolved "https://registry.npmjs.org/strip-ansi/-/strip-ansi-2.0.1.tgz#df62c1aa94ed2f114e1d0f21fd1d50482b79a60e"
+  dependencies:
+    ansi-regex "^1.0.0"

__tests__/fixtures/upgrade/with-subdep/package.json

@@ -0,0 +1,6 @@
+{
+  "dependencies": {
+    "strip-ansi": "^2.0.1",
+    "array-union": "^1.0.1"
+  }
+}

__tests__/fixtures/upgrade/with-subdep/yarn.lock

@@ -0,0 +1,18 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
+# yarn lockfile v1
+ansi-regex@^1.0.0:
+  version "1.0.0"
+  resolved "https://registry.npmjs.org/ansi-regex/-/ansi-regex-1.0.0.tgz#54c7ce13af71e436348666484c44516ab9bc144e"
+array-union@^1.0.1:
+  version "1.0.1"
+  resolved "https://registry.npmjs.org/array-union/-/array-union-1.0.1.tgz#4d410fc8395cb247637124bade9e3f547d5d55f2"
+  dependencies:
+    array-uniq "^1.0.1"
+array-uniq@^1.0.1:
+  version "1.0.1"
+  resolved "https://registry.npmjs.org/array-uniq/-/array-uniq-1.0.1.tgz#25e1d96853d7f6f77cecf693f86cac4052046790"
+strip-ansi@^2.0.1:
+  version "2.0.1"
+  resolved "https://registry.npmjs.org/strip-ansi/-/strip-ansi-2.0.1.tgz#df62c1aa94ed2f114e1d0f21fd1d50482b79a60e"
+  dependencies:
+    ansi-regex "^1.0.0"

src/cli/commands/upgrade-interactive.js

@@ -6,9 +6,10 @@ import type Config from '../../config.js';
 import inquirer from 'inquirer';
 import Lockfile from '../../lockfile';
 import {Add} from './add.js';
-import {getOutdated} from './upgrade.js';
+import {getOutdated, cleanLockfile} from './upgrade.js';
 import colorForVersions from '../../util/color-for-versions';
 import colorizeDiff from '../../util/colorize-diff.js';
+import {Install} from './install.js';
 
 const path = require('path');
 
@@ -159,6 +160,8 @@ export async function run(config: Config, reporter: Reporter, flags: Object, arg
       flags.workspaceRootIsCwd = false;
       const deps = answers.filter(isHint(hint));
       if (deps.length) {
+        const install = new Install(flags, config, reporter, lockfile);
+        const {requests: packagePatterns} = await install.fetchRequestFromCwd();
         const depsByWorkspace = deps.reduce((acc, dep) => {
           const {workspaceLoc} = dep;
           const xs = acc[workspaceLoc] || [];
@@ -167,9 +170,7 @@ export async function run(config: Config, reporter: Reporter, flags: Object, arg
         }, {});
         for (const loc of Object.keys(depsByWorkspace)) {
           const patterns = depsByWorkspace[loc].map(getPattern);
-          for (const pattern of patterns) {
-            lockfile.removePattern(pattern);
-          }
+          cleanLockfile(lockfile, deps, packagePatterns, reporter);
           reporter.info(reporter.lang('updateInstalling', getNameFromHint(hint)));
           config.cwd = path.resolve(path.dirname(loc));
           const add = new Add(patterns, flags, config, reporter, lockfile);

src/cli/commands/upgrade.js

@@ -3,6 +3,7 @@
 import type {Dependency} from '../../types.js';
 import type {Reporter} from '../../reporters/index.js';
 import type Config from '../../config.js';
+import type {DependencyRequestPatterns} from '../../types.js';
 import {Add} from './add.js';
 import Lockfile from '../../lockfile';
 import PackageRequest from '../../package-request.js';
@@ -14,23 +15,46 @@ import {Install} from './install.js';
 const basicSemverOperatorRegex = new RegExp('^(\\^|~|>|<=|>=)?[^ |&,]+$');
 
 // used to detect if a passed parameter is a scope or a package name.
-const validScopeRegex = /^@[a-zA-Z0-9-][a-zA-Z0-9_.-]*\/$/g;
+const validScopeRegex = /^@[a-zA-Z0-9-][a-zA-Z0-9_.-]*\/$/;
 
 // If specific versions were requested for packages, override what getOutdated reported as the latest to install
 // Also add ones that are missing, since the requested packages may not have been outdated at all.
-function setUserRequestedPackageVersions(deps: Array<Dependency>, args: Array<string>) {
+function setUserRequestedPackageVersions(
+  deps: Array<Dependency>,
+  args: Array<string>,
+  latest: boolean,
+  packagePatterns,
+  reporter: Reporter,
+) {
   args.forEach(requestedPattern => {
-    const normalized = normalizePattern(requestedPattern);
-    const newPattern = `${normalized.name}@${normalized.range}`;
     let found = false;
+    let normalized = normalizePattern(requestedPattern);
+
+    // if the user specified a package name without a version range, then that implies "latest"
+    // but if the latest flag is not passed then we need to use the version range from package.json
+    if (!normalized.hasVersion && !latest) {
+      packagePatterns.forEach(packagePattern => {
+        const packageNormalized = normalizePattern(packagePattern.pattern);
+        if (packageNormalized.name === normalized.name) {
+          normalized = packageNormalized;
+        }
+      });
+    }
+
+    const newPattern = `${normalized.name}@${normalized.range}`;
 
+    // if this dependency is already in the outdated list,
+    // just update the upgradeTo to whatever version the user requested.
     deps.forEach(dep => {
       if (normalized.hasVersion && dep.name === normalized.name) {
         found = true;
         dep.upgradeTo = newPattern;
+        reporter.verbose(reporter.lang('verboseUpgradeBecauseRequested', requestedPattern, newPattern));
       }
     });
 
+    // if this dependency was not in the outdated list,
+    // then add a new entry
     if (normalized.hasVersion && !found) {
       deps.push({
         name: normalized.name,
@@ -44,10 +68,81 @@ function setUserRequestedPackageVersions(deps: Array<Dependency>, args: Array<st
         workspaceName: '',
         workspaceLoc: '',
       });
+      reporter.verbose(reporter.lang('verboseUpgradeBecauseRequested', requestedPattern, newPattern));
     }
   });
 }
 
+// this function attempts to determine the range operator on the semver range.
+// this will only handle the simple cases of a semver starting with '^', '~', '>', '>=', '<=', or an exact version.
+// "exotic" semver ranges will not be handled.
+function getRangeOperator(version): string {
+  const result = basicSemverOperatorRegex.exec(version);
+  return result ? result[1] || '' : '^';
+}
+
+// Attempt to preserve the range operator from the package.json specified semver range.
+// If an explicit operator was specified using --exact, --tilde, --caret, then that will take precedence.
+function buildPatternToUpgradeTo(dep, flags): string {
+  if (dep.latest === 'exotic') {
+    return dep.url;
+  }
+
+  const toLatest = flags.latest;
+  const toVersion = toLatest ? dep.latest : dep.range;
+  let rangeOperator = '';
+
+  if (toLatest) {
+    if (flags.caret) {
+      rangeOperator = '^';
+    } else if (flags.tilde) {
+      rangeOperator = '~';
+    } else if (flags.exact) {
+      rangeOperator = '';
+    } else {
+      rangeOperator = getRangeOperator(dep.range);
+    }
+  }
+
+  return `${dep.name}@${rangeOperator}${toVersion}`;
+}
+
+function scopeFilter(flags: Object, dep: Dependency): boolean {
+  if (validScopeRegex.test(flags.scope)) {
+    return dep.name.startsWith(flags.scope);
+  }
+  return true;
+}
+
+// Remove deps being upgraded from the lockfile, or else Add will use the already-installed version
+// instead of the latest for the range.
+// We do this recursively so that when Yarn installs the potentially updated transitive deps,
+// it may upgrade them too instead of just using the "locked" version from the lockfile.
+// Transitive dependencies that are also a direct dependency are skipped.
+export function cleanLockfile(
+  lockfile: Lockfile,
+  deps: Array<Dependency>,
+  packagePatterns: DependencyRequestPatterns,
+  reporter: Reporter,
+) {
+  function cleanDepFromLockfile(pattern: string, depth: number) {
+    const lockManifest = lockfile.getLocked(pattern);
+    if (!lockManifest || (depth > 1 && packagePatterns.some(packagePattern => packagePattern.pattern === pattern))) {
+      reporter.verbose(reporter.lang('verboseUpgradeNotUnlocking', pattern));
+      return;
+    }
+
+    const dependencies = Object.assign({}, lockManifest.dependencies || {}, lockManifest.optionalDependencies || {});
+    const depPatterns = Object.keys(dependencies).map(key => `${key}@${dependencies[key]}`);
+    reporter.verbose(reporter.lang('verboseUpgradeUnlocking', pattern));
+    lockfile.removePattern(pattern);
+    depPatterns.forEach(pattern => cleanDepFromLockfile(pattern, depth + 1));
+  }
+
+  const patterns = deps.map(dep => dep.upgradeTo);
+  patterns.forEach(pattern => cleanDepFromLockfile(pattern, 1));
+}
+
 export function setFlags(commander: Object) {
   commander.usage('upgrade [flags]');
   commander.option('-S, --scope <scope>', 'upgrade packages under the specified scope');
@@ -71,30 +166,27 @@ export function hasWrapper(commander: Object, args: Array<string>): boolean {
 export const requireLockfile = true;
 
 export async function run(config: Config, reporter: Reporter, flags: Object, args: Array<string>): Promise<void> {
-  const lockfile = await Lockfile.fromDirectory(config.lockfileFolder);
-  const deps = await getOutdated(config, reporter, flags, lockfile, args);
-
-  // do not pass the --latest flag to add, otherwise it may ignore the version ranges we already determined.
+  let addArgs = [];
+  const upgradeAll = args.length === 0;
   const addFlags = Object.assign({}, flags, {
     force: true,
-    latest: false,
     ignoreWorkspaceRootCheck: true,
     workspaceRootIsCwd: config.cwd === config.lockfileFolder,
   });
+  const lockfile = await Lockfile.fromDirectory(config.lockfileFolder, reporter);
+  const deps = await getOutdated(config, reporter, flags, lockfile, args);
+  const install = new Install(flags, config, reporter, lockfile);
+  const {requests: packagePatterns} = await install.fetchRequestFromCwd();
 
-  setUserRequestedPackageVersions(deps, args);
+  setUserRequestedPackageVersions(deps, args, flags.latest, packagePatterns, reporter);
+  cleanLockfile(lockfile, deps, packagePatterns, reporter);
+  addArgs = deps.map(dep => dep.upgradeTo);
 
-  if (!deps.length) {
-    reporter.success(reporter.lang('allDependenciesUpToDate'));
-    return;
+  if (flags.scope && validScopeRegex.test(flags.scope)) {
+    addArgs = addArgs.filter(depName => depName.startsWith(flags.scope));
   }
 
-  // remove deps being upgraded from the lockfile, or else Add will use the already-installed version
-  // instead of the latest for the range.
-  deps.forEach(dep => lockfile.removePattern(dep.upgradeTo));
-
-  const addArgs = deps.map(dep => dep.upgradeTo);
-  const add = new Add(addArgs, addFlags, config, reporter, lockfile);
+  const add = new Add(addArgs, addFlags, config, reporter, upgradeAll ? new Lockfile() : lockfile);
   await add.init();
 }
 
@@ -107,40 +199,7 @@ export async function getOutdated(
 ): Promise<Array<Dependency>> {
   const install = new Install(flags, config, reporter, lockfile);
   const outdatedFieldName = flags.latest ? 'latest' : 'wanted';
-
-  // this function attempts to determine the range operator on the semver range.
-  // this will only handle the simple cases of a semver starting with '^', '~', '>', '>=', '<=', or an exact version.
-  // "exotic" semver ranges will not be handled.
-  const getRangeOperator = version => {
-    const result = basicSemverOperatorRegex.exec(version);
-    return result ? result[1] || '' : '^';
-  };
-
-  // Attempt to preserve the range operator from the package.json specified semver range.
-  // If an explicit operator was specified using --exact, --tilde, --caret, then that will take precedence.
-  const buildPatternToUpgradeTo = (dep, flags) => {
-    if (dep.latest === 'exotic') {
-      return dep.url;
-    }
-
-    const toLatest = flags.latest;
-    const toVersion = toLatest ? dep.latest : dep.range;
-    let rangeOperator = '';
-
-    if (toLatest) {
-      if (flags.caret) {
-        rangeOperator = '^';
-      } else if (flags.tilde) {
-        rangeOperator = '~';
-      } else if (flags.exact) {
-        rangeOperator = '';
-      } else {
-        rangeOperator = getRangeOperator(dep.range);
-      }
-    }
-
-    return `${dep.name}@${rangeOperator}${toVersion}`;
-  };
+  const updateAll = patterns.length === 0;
 
   // ensure scope is of the form `@scope/`
   const normalizeScope = function() {
@@ -159,14 +218,6 @@ export async function getOutdated(
     return dep.current !== dep[outdatedFieldName];
   };
 
-  const scopeFilter = function(dep: Dependency): boolean {
-    if (validScopeRegex.test(flags.scope)) {
-      return dep.name.startsWith(flags.scope);
-    }
-
-    return true;
-  };
-
   if (!flags.latest) {
     // these flags only have an affect when --latest is used
     flags.tilde = false;
@@ -176,10 +227,21 @@ export async function getOutdated(
 
   normalizeScope();
 
-  const deps = (await PackageRequest.getOutdatedPackages(lockfile, install, config, reporter, patterns, flags))
+  const deps = (await PackageRequest.getOutdatedPackages(
+    lockfile,
+    install,
+    config,
+    reporter,
+    patterns,
+    flags,
+    updateAll,
+  ))
     .filter(versionFilter)
-    .filter(scopeFilter);
-  deps.forEach(dep => (dep.upgradeTo = buildPatternToUpgradeTo(dep, flags)));
+    .filter(scopeFilter.bind(this, flags));
+  deps.forEach(dep => {
+    dep.upgradeTo = buildPatternToUpgradeTo(dep, flags);
+    reporter.verbose(reporter.lang('verboseUpgradeBecauseOutdated', dep.name, dep.upgradeTo));
+  });
 
   return deps;
 }