Skip to content

Commit

Permalink
fix: npm update --save
Browse files Browse the repository at this point in the history
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
  • Loading branch information
ruyadorno committed Jan 7, 2022
1 parent 0d3cf8b commit f7456da
Show file tree
Hide file tree
Showing 4 changed files with 226 additions and 8 deletions.
2 changes: 1 addition & 1 deletion workspaces/arborist/lib/arborist/build-ideal-tree.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
61 changes: 54 additions & 7 deletions workspaces/arborist/lib/arborist/reify.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
28 changes: 28 additions & 0 deletions workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
143 changes: 143 additions & 0 deletions workspaces/arborist/test/arborist/reify.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()
})

0 comments on commit f7456da

Please sign in to comment.