fix: npm update --save

Previously `npm update` was not respecting the `save` option, it
would be impossible for users to use `npm update` and automatically
update their `package.json` files.

This fixes it by adding extra steps on `Arborist.reify._saveIdealTree`
to read direct dependencies of any `package.json` and update them as
needed when reifying using the `update` and `save` options.

Fixes: #708
Fixes: #2704
Relates to: npm/feedback#270

Author: ruyadorno

workspaces/arborist/lib/arborist/build-ideal-tree.js

@@ -41,7 +41,7 @@ const _complete = Symbol('complete')
 const _depsSeen = Symbol('depsSeen')
 const _depsQueue = Symbol('depsQueue')
 const _currentDep = Symbol('currentDep')
-const _updateAll = Symbol('updateAll')
+const _updateAll = Symbol.for('updateAll')
 const _mutateTree = Symbol('mutateTree')
 const _flagsSuspect = Symbol.for('flagsSuspect')
 const _workspaces = Symbol.for('workspaces')

workspaces/arborist/lib/arborist/reify.js

@@ -58,6 +58,8 @@ const _bundleUnpacked = Symbol('bundleUnpacked')
 const _bundleMissing = Symbol('bundleMissing')
 const _reifyNode = Symbol.for('reifyNode')
 const _extractOrLink = Symbol('extractOrLink')
+const _updateAll = Symbol.for('updateAll')
+const _updateNames = Symbol.for('updateNames')
 // defined by rebuild mixin
 const _checkBins = Symbol.for('checkBins')
 const _symlink = Symbol('symlink')
@@ -1148,13 +1150,8 @@ module.exports = cls => class Reifier extends cls {
     process.emit('time', 'reify:save')
 
     const updatedTrees = new Set()
-
-    // resolvedAdd is the list of user add requests, but with names added
-    // to things like git repos and tarball file/urls.  However, if the
-    // user requested 'foo@', and we have a foo@file:../foo, then we should
-    // end up saving the spec we actually used, not whatever they gave us.
-    if (this[_resolvedAdd].length) {
-      for (const { name, tree: addTree } of this[_resolvedAdd]) {
+    const updateNodes = nodes => {
+      for (const { name, tree: addTree } of nodes) {
         // addTree either the root, or a workspace
         const edge = addTree.edgesOut.get(name)
         const pkg = addTree.package
@@ -1259,6 +1256,56 @@ module.exports = cls => class Reifier extends cls {
       }
     }
 
+    // helper that retrieves an array of nodes that were
+    // potentially updated during the reify process, in order
+    // to limit the number of nodes to check and update, only
+    // select nodes from the inventory that are direct deps
+    // of a given package.json (project root or a workspace)
+    // and in ase of using a list of `names`, restrict nodes
+    // to only names that are found in this list
+    const retrieveUpdatedNodes = names => {
+      const filterDirectDependencies = node =>
+        !node.isRoot && node.resolveParent.isRoot
+        && (!names || names.includes(node.name))
+      const directDeps = this.idealTree.inventory
+        .filter(filterDirectDependencies)
+
+      // traverses the list of direct dependencies and collect all nodes
+      // to be updated, since any of them might have changed during reify
+      const nodes = []
+      for (const node of directDeps) {
+        for (const edgeIn of node.edgesIn) {
+          nodes.push({
+            name: node.name,
+            tree: edgeIn.from.target,
+          })
+        }
+      }
+      return nodes
+    }
+
+    // when using update all alongside with save, we'll make
+    // sure to refresh every dependency of the root idealTree
+    if (this[_updateAll] && options.save) {
+      const nodes = retrieveUpdatedNodes()
+      updateNodes(nodes)
+    } else {
+      // resolvedAdd is the list of user add requests, but with names added
+      // to things like git repos and tarball file/urls.  However, if the
+      // user requested 'foo@', and we have a foo@file:../foo, then we should
+      // end up saving the spec we actually used, not whatever they gave us.
+      if (this[_resolvedAdd].length) {
+        updateNodes(this[_resolvedAdd])
+      }
+
+      // if updating given dependencies by name, restrict the list of
+      // nodes to check to only those currently in _updateNames
+      if (this[_updateNames].length && options.save) {
+        const nodes = retrieveUpdatedNodes(this[_updateNames])
+        updateNodes(nodes)
+      }
+    }
+
     // preserve indentation, if possible
     const {
       [Symbol.for('indent')]: indent,

workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs

@@ -32468,6 +32468,34 @@ exports[`test/arborist/reify.js TAP save complete lockfile on update-all > shoul
 
 `
 
+exports[`test/arborist/reify.js TAP save package.json on update should update both package.json and package-lock.json using save=true > should update lockfile with same dep range from now updated package.json 1`] = `
+{
+  "name": "tap-testdir-reify-save-package.json-on-update-should-update-both-package.json-and-package-lock.json-using-save-true",
+  "lockfileVersion": 2,
+  "requires": true,
+  "packages": {
+    "": {
+      "dependencies": {
+        "abbrev": "^1.1.1"
+      }
+    },
+    "node_modules/abbrev": {
+      "version": "1.1.1",
+      "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
+      "integrity": "sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q=="
+    }
+  },
+  "dependencies": {
+    "abbrev": {
+      "version": "1.1.1",
+      "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
+      "integrity": "sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q=="
+    }
+  }
+}
+
+`
+
 exports[`test/arborist/reify.js TAP save proper lockfile with bins when upgrading lockfile complete=false > should upgrade, with bins in place 1`] = `
 {
   "name": "tap-testdir-reify-save-proper-lockfile-with-bins-when-upgrading-lockfile-complete-false",

workspaces/arborist/test/arborist/reify.js

@@ -2432,3 +2432,146 @@ t.test('add local dep with existing dev + peer/optional', async t => {
   t.equal(tree.children.get('abbrev').resolved, 'file:../../dep', 'resolved')
   t.equal(tree.children.size, 1, 'children')
 })
+
+t.test('save package.json on update', t => {
+  const pjson = {
+    dependencies: {
+      abbrev: '^1.0.9',
+    },
+  }
+  const expectedUsingSaveOption = {
+    dependencies: {
+      abbrev: '^1.1.1',
+    },
+  }
+  const plock = {
+    name: 'tap-testdir-reify-save-package.json-on-update-should-update-package-lock.json',
+    version: '1.0.0',
+    lockfileVersion: 2,
+    requires: true,
+    packages: {
+      '': {
+        name: 'a',
+        version: '1.0.0',
+        license: 'MIT',
+        dependencies: {
+          abbrev: '^1.0.9',
+        },
+      },
+      'node_modules/abbrev': {
+        version: '1.0.9',
+        resolved: 'https://registry.npmjs.org/abbrev/-/abbrev-1.0.9.tgz',
+        integrity: 'sha1-kbR5JYinc4wl813W9jdSovh3YTU=',
+      },
+    },
+    dependencies: {
+      abbrev: {
+        version: '1.0.9',
+        resolved: 'https://registry.npmjs.org/abbrev/-/abbrev-1.0.9.tgz',
+        integrity: 'sha1-kbR5JYinc4wl813W9jdSovh3YTU=',
+      },
+    },
+  }
+
+  t.test('should not touch package.json on update-all', async t => {
+    const path = t.testdir({
+      'package.json': JSON.stringify(pjson),
+    })
+
+    await reify(path, { update: true })
+
+    t.same(require(resolve(path, 'package.json')), pjson,
+      'should not have changed package.json file on update all')
+  })
+
+  t.test('should update package-lock.json', async t => {
+    const path = t.testdir({
+      'package.json': JSON.stringify(pjson),
+      'package-lock.json': JSON.stringify(plock),
+    })
+
+    await reify(path, { update: true })
+
+    t.same(require(resolve(path, 'package.json')), pjson,
+      'should not have changed package.json file on update all')
+
+    // in a package-lock the version gets updated even though the
+    // package.json dependency info hasn't been updated
+    const lock = require(resolve(path, 'package-lock.json'))
+    const lockedVersion = lock.packages['node_modules/abbrev'].version
+    t.equal(lockedVersion, '1.1.1',
+      'should update locked versions on packages entries')
+    const depRange = lock.packages[''].dependencies.abbrev
+    t.equal(depRange, '^1.0.9',
+      'should have same version range described in package.json')
+  })
+
+  t.test('should not touch package.json on named update', async t => {
+    const path = t.testdir({
+      'package.json': JSON.stringify(pjson),
+    })
+
+    await reify(path, { update: { names: ['abbrev'] } })
+
+    t.same(require(resolve(path, 'package.json')), pjson,
+      'should not have changed package.json file on named update')
+  })
+
+  t.test('should save to package.json when using save=true', async t => {
+    const path = t.testdir({
+      'package.json': JSON.stringify(pjson),
+    })
+
+    await reify(path, { update: true, save: true })
+
+    t.same(require(resolve(path, 'package.json')), expectedUsingSaveOption,
+      'should save pkg change to package.json file on update all')
+  })
+
+  t.test('should save to package.json on named update using save=true', async t => {
+    const path = t.testdir({
+      'package.json': JSON.stringify(pjson),
+    })
+
+    await reify(path, { update: { names: ['abbrev'] }, save: true })
+
+    t.same(require(resolve(path, 'package.json')), expectedUsingSaveOption,
+      'should save pkg change to package.json file on named update')
+  })
+
+  t.test('should update both package.json and package-lock.json using save=true', async t => {
+    const path = t.testdir({
+      'package.json': JSON.stringify(pjson),
+      'package-lock.json': JSON.stringify(plock),
+    })
+
+    await reify(path, { update: true, save: true })
+
+    t.same(require(resolve(path, 'package.json')), expectedUsingSaveOption,
+      'should change package.json file on update all along with lockfile')
+
+    t.matchSnapshot(fs.readFileSync(resolve(path, 'package-lock.json'), 'utf8'),
+      'should update lockfile with same dep range from now updated package.json')
+  })
+
+  t.test('should save to multiple package.json when using save=true', async t => {
+    const path = t.testdir({
+      'package.json': JSON.stringify({
+        ...pjson,
+        workspaces: ['a'],
+      }),
+      a: {
+        'package.json': JSON.stringify(pjson),
+      },
+    })
+
+    await reify(path, { update: true, save: true })
+
+    t.match(require(resolve(path, 'package.json')), expectedUsingSaveOption,
+      'should save pkg change to all package.json files on update all')
+    t.same(require(resolve(path, 'a', 'package.json')), expectedUsingSaveOption,
+      'should save workspace pkg change to all package.json files on update all')
+  })
+
+  t.end()
+})