From f7456da5fb761080f7e6ebd6a72fe15420b875b0 Mon Sep 17 00:00:00 2001 From: Ruy Adorno Date: Fri, 7 Jan 2022 17:53:13 -0500 Subject: [PATCH] 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: https://github.com/npm/cli/issues/708 Fixes: https://github.com/npm/cli/issues/2704 Relates to: https://github.com/npm/feedback/discussions/270 --- .../arborist/lib/arborist/build-ideal-tree.js | 2 +- workspaces/arborist/lib/arborist/reify.js | 61 +++++++- .../test/arborist/reify.js.test.cjs | 28 ++++ workspaces/arborist/test/arborist/reify.js | 143 ++++++++++++++++++ 4 files changed, 226 insertions(+), 8 deletions(-) diff --git a/workspaces/arborist/lib/arborist/build-ideal-tree.js b/workspaces/arborist/lib/arborist/build-ideal-tree.js index 899d92ca937cc..e21245f861a43 100644 --- a/workspaces/arborist/lib/arborist/build-ideal-tree.js +++ b/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') diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index 547e54ac37670..87f521de5483b 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/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, diff --git a/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs b/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs index 7a6a3e714ba51..a38242140a231 100644 --- a/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs +++ b/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", diff --git a/workspaces/arborist/test/arborist/reify.js b/workspaces/arborist/test/arborist/reify.js index c8c4cb137d424..50b1aebf441e8 100644 --- a/workspaces/arborist/test/arborist/reify.js +++ b/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() +})