Skip to content

Commit 49d9a3d

Browse files
ruyadornoruyadorno
committed
fix: addressed code review
- added more tests scenarios - fixed some typos
1 parent 3bb7bac commit 49d9a3d

15 files changed

Lines changed: 734 additions & 218 deletions

File tree

‎docs/content/using-npm/config.md‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1326,16 +1326,16 @@ The base URL of the npm registry.
13261326

13271327
#### `save`
13281328

1329-
* Default: true unless when using `npm update` or `npm dedupe` where it
1329+
* Default: `true` unless when using `npm update` or `npm dedupe` where it
13301330
defaults to `false`
13311331
* Type: Boolean
13321332

1333-
Save installed packages to a package.json file as dependencies.
1333+
Save installed packages to a `package.json` file as dependencies.
13341334

13351335
When used with the `npm rm` command, removes the dependency from
1336-
package.json.
1336+
`package.json`.
13371337

1338-
Will also prevent writing to package-lock.json if set to `false`.
1338+
Will also prevent writing to `package-lock.json` if set to `false`.
13391339

13401340
<!-- automatically generated, do not edit manually -->
13411341
<!-- see lib/utils/config/definitions.js -->

‎lib/commands/update.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ class Update extends ArboristWorkspaceCmd {
4141
? global
4242
: this.npm.prefix
4343

44-
// In the context of `npm upgrade` the save
44+
// In the context of `npm update` the save
4545
// config value should default to `false`
4646
const save = this.npm.config.isDefault('save')
4747
? false

‎lib/utils/config/definitions.js‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1583,18 +1583,18 @@ define('registry', {
15831583

15841584
define('save', {
15851585
default: true,
1586-
defaultDescription: `true unless when using \`npm update\` or
1586+
defaultDescription: `\`true\` unless when using \`npm update\` or
15871587
\`npm dedupe\` where it defaults to \`false\``,
15881588
usage: '-S|--save|--no-save|--save-prod|--save-dev|--save-optional|--save-peer',
15891589
type: Boolean,
15901590
short: 'S',
15911591
description: `
1592-
Save installed packages to a package.json file as dependencies.
1592+
Save installed packages to a \`package.json\` file as dependencies.
15931593
15941594
When used with the \`npm rm\` command, removes the dependency from
1595-
package.json.
1595+
\`package.json\`.
15961596
1597-
Will also prevent writing to package-lock.json if set to \`false\`.
1597+
Will also prevent writing to \`package-lock.json\` if set to \`false\`.
15981598
`,
15991599
flatten,
16001600
})

‎smoke-tests/index.js‎

Lines changed: 48 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -268,14 +268,55 @@ t.test('npm pkg', async t => {
268268
)
269269
})
270270

271+
t.test('npm update --no-save --no-package-lock', async t => {
272+
// setup, manually reset dep value
273+
await exec(`${npmBin} pkg set dependencies.abbrev==1.0.4`)
274+
await exec(`${npmBin} install`)
275+
await exec(`${npmBin} pkg set dependencies.abbrev=^1.0.4`)
276+
277+
const cmd = `${npmBin} update --no-save --no-package-lock`
278+
await exec(cmd)
279+
280+
t.equal(
281+
JSON.parse(readFile('package.json')).dependencies.abbrev,
282+
'^1.0.4',
283+
'should have expected update --no-save --no-package-lock package.json result'
284+
)
285+
t.equal(
286+
JSON.parse(readFile('package-lock.json')).packages['node_modules/abbrev'].version,
287+
'1.0.4',
288+
'should have expected update --no-save --no-package-lock lockfile result'
289+
)
290+
})
291+
292+
t.test('npm update --no-save', async t => {
293+
const cmd = `${npmBin} update --no-save`
294+
await exec(cmd)
295+
296+
t.equal(
297+
JSON.parse(readFile('package.json')).dependencies.abbrev,
298+
'^1.0.4',
299+
'should have expected update --no-save package.json result'
300+
)
301+
t.equal(
302+
JSON.parse(readFile('package-lock.json')).packages['node_modules/abbrev'].version,
303+
'1.1.1',
304+
'should have expected update --no-save lockfile result'
305+
)
306+
})
307+
271308
t.test('npm update --save', async t => {
272309
const cmd = `${npmBin} update --save`
273-
const cmdRes = await exec(cmd)
310+
await exec(cmd)
274311

275-
t.matchSnapshot(cmdRes.replace(/in.*s/, ''),
276-
'should have expected update --save reify output')
277-
t.matchSnapshot(readFile('package.json'),
278-
'should have expected update --save package.json result')
279-
t.matchSnapshot(readFile('package-lock.json'),
280-
'should have expected update --save lockfile result')
312+
t.equal(
313+
JSON.parse(readFile('package.json')).dependencies.abbrev,
314+
'^1.1.1',
315+
'should have expected update --save package.json result'
316+
)
317+
t.equal(
318+
JSON.parse(readFile('package-lock.json')).packages['node_modules/abbrev'].version,
319+
'1.1.1',
320+
'should have expected update --save lockfile result'
321+
)
281322
})

‎tap-snapshots/smoke-tests/index.js.test.cjs‎

Lines changed: 0 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -666,64 +666,6 @@ removed 1 package
666666
667667
`
668668

669-
exports[`smoke-tests/index.js TAP npm update --save > should have expected update --save lockfile result 1`] = `
670-
{
671-
"name": "project",
672-
"version": "1.0.0",
673-
"lockfileVersion": 2,
674-
"requires": true,
675-
"packages": {
676-
"": {
677-
"name": "project",
678-
"version": "1.0.0",
679-
"license": "ISC",
680-
"dependencies": {
681-
"abbrev": "^1.1.1"
682-
}
683-
},
684-
"node_modules/abbrev": {
685-
"version": "1.1.1",
686-
"resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
687-
"integrity": "sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q=="
688-
}
689-
},
690-
"dependencies": {
691-
"abbrev": {
692-
"version": "1.1.1",
693-
"resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
694-
"integrity": "sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q=="
695-
}
696-
}
697-
}
698-
699-
`
700-
701-
exports[`smoke-tests/index.js TAP npm update --save > should have expected update --save package.json result 1`] = `
702-
{
703-
"name": "project",
704-
"version": "1.0.0",
705-
"description": "",
706-
"main": "index.js",
707-
"scripts": {
708-
"test": "echo /"Error: no test specified/" && exit 1",
709-
"hello": "echo Hello"
710-
},
711-
"keywords": [],
712-
"author": "",
713-
"license": "ISC",
714-
"dependencies": {
715-
"abbrev": "^1.1.1"
716-
}
717-
}
718-
719-
`
720-
721-
exports[`smoke-tests/index.js TAP npm update --save > should have expected update --save reify output 1`] = `
722-
723-
up to date
724-
725-
`
726-
727669
exports[`smoke-tests/index.js TAP npm update dep > should have expected update lockfile result 1`] = `
728670
{
729671
"name": "project",

‎tap-snapshots/test/lib/utils/config/definitions.js.test.cjs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1406,16 +1406,16 @@ The base URL of the npm registry.
14061406
exports[`test/lib/utils/config/definitions.js TAP > config description for save 1`] = `
14071407
#### \`save\`
14081408
1409-
* Default: true unless when using \`npm update\` or \`npm dedupe\` where it
1409+
* Default: \`true\` unless when using \`npm update\` or \`npm dedupe\` where it
14101410
defaults to \`false\`
14111411
* Type: Boolean
14121412
1413-
Save installed packages to a package.json file as dependencies.
1413+
Save installed packages to a \`package.json\` file as dependencies.
14141414
14151415
When used with the \`npm rm\` command, removes the dependency from
1416-
package.json.
1416+
\`package.json\`.
14171417
1418-
Will also prevent writing to package-lock.json if set to \`false\`.
1418+
Will also prevent writing to \`package-lock.json\` if set to \`false\`.
14191419
`
14201420

14211421
exports[`test/lib/utils/config/definitions.js TAP > config description for save-bundle 1`] = `

‎tap-snapshots/test/lib/utils/config/describe-all.js.test.cjs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1200,16 +1200,16 @@ The base URL of the npm registry.
12001200
12011201
#### \`save\`
12021202
1203-
* Default: true unless when using \`npm update\` or \`npm dedupe\` where it
1203+
* Default: \`true\` unless when using \`npm update\` or \`npm dedupe\` where it
12041204
defaults to \`false\`
12051205
* Type: Boolean
12061206
1207-
Save installed packages to a package.json file as dependencies.
1207+
Save installed packages to a \`package.json\` file as dependencies.
12081208
12091209
When used with the \`npm rm\` command, removes the dependency from
1210-
package.json.
1210+
\`package.json\`.
12111211
1212-
Will also prevent writing to package-lock.json if set to \`false\`.
1212+
Will also prevent writing to \`package-lock.json\` if set to \`false\`.
12131213
12141214
<!-- automatically generated, do not edit manually -->
12151215
<!-- see lib/utils/config/definitions.js -->

‎workspaces/arborist/lib/arborist/reify.js‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1142,21 +1142,25 @@ module.exports = cls => class Reifier extends cls {
11421142
// for install failures. Those still end up in the shrinkwrap, so we
11431143
// save it first, then prune out the optional trash, and then return it.
11441144

1145-
const skipSave = options.save === false
1146-
const save = !skipSave
1145+
const save = !(options.save === false)
1146+
1147+
// we check for updates in order to make sure we run save ideal tree
1148+
// even though save=false since we want `npm update` to be able to
1149+
// write to package-lock files by default
11471150
const hasUpdates = this[_updateAll] || this[_updateNames].length
11481151

11491152
// we're going to completely skip save ideal tree in case of a global or
11501153
// dry-run install and also if the save option is set to false, EXCEPT for
11511154
// update since the expected behavior for npm7+ is for update to
11521155
// NOT save to package.json, we make that exception since we still want
1153-
// saveIdealTree to be able to write the lockfile
1154-
const skipSaveIdealTree =
1155-
(skipSave && !hasUpdates)
1156+
// saveIdealTree to be able to write the lockfile by default.
1157+
const saveIdealTree = !(
1158+
(!save && !hasUpdates)
11561159
|| this[_global]
11571160
|| this[_dryRun]
1161+
)
11581162

1159-
if (skipSaveIdealTree) {
1163+
if (!saveIdealTree) {
11601164
return false
11611165
}
11621166

0 commit comments

Comments
 (0)