fix: addressed code review

- added more tests scenarios
- fixed some typos

docs/content/using-npm/config.md

@@ -1326,16 +1326,16 @@ The base URL of the npm registry.
 
 #### `save`
 
-* Default: true unless when using `npm update` or `npm dedupe` where it
+* Default: `true` unless when using `npm update` or `npm dedupe` where it
   defaults to `false`
 * Type: Boolean
 
-Save installed packages to a package.json file as dependencies.
+Save installed packages to a `package.json` file as dependencies.
 
 When used with the `npm rm` command, removes the dependency from
-package.json.
+`package.json`.
 
-Will also prevent writing to package-lock.json if set to `false`.
+Will also prevent writing to `package-lock.json` if set to `false`.
 
 <!-- automatically generated, do not edit manually -->
 <!-- see lib/utils/config/definitions.js -->

lib/commands/update.js

@@ -41,7 +41,7 @@ class Update extends ArboristWorkspaceCmd {
       ? global
       : this.npm.prefix
 
-    // In the context of `npm upgrade` the save
+    // In the context of `npm update` the save
     // config value should default to `false`
     const save = this.npm.config.isDefault('save')
       ? false

lib/utils/config/definitions.js

@@ -1583,18 +1583,18 @@ define('registry', {
 
 define('save', {
   default: true,
-  defaultDescription: `true unless when using \`npm update\` or
+  defaultDescription: `\`true\` unless when using \`npm update\` or
   \`npm dedupe\` where it defaults to \`false\``,
   usage: '-S|--save|--no-save|--save-prod|--save-dev|--save-optional|--save-peer',
   type: Boolean,
   short: 'S',
   description: `
-    Save installed packages to a package.json file as dependencies.
+    Save installed packages to a \`package.json\` file as dependencies.
 
     When used with the \`npm rm\` command, removes the dependency from
-    package.json.
+    \`package.json\`.
 
-    Will also prevent writing to package-lock.json if set to \`false\`.
+    Will also prevent writing to \`package-lock.json\` if set to \`false\`.
   `,
   flatten,
 })

smoke-tests/index.js

@@ -268,14 +268,55 @@ t.test('npm pkg', async t => {
   )
 })
 
+t.test('npm update --no-save --no-package-lock', async t => {
+  // setup, manually reset dep value
+  await exec(`${npmBin} pkg set "dependencies.abbrev==1.0.4"`)
+  await exec(`${npmBin} install`)
+  await exec(`${npmBin} pkg set "dependencies.abbrev=^1.0.4"`)
+
+  const cmd = `${npmBin} update --no-save --no-package-lock`
+  await exec(cmd)
+
+  t.equal(
+    JSON.parse(readFile('package.json')).dependencies.abbrev,
+    '^1.0.4',
+    'should have expected update --no-save --no-package-lock package.json result'
+  )
+  t.equal(
+    JSON.parse(readFile('package-lock.json')).packages['node_modules/abbrev'].version,
+    '1.0.4',
+    'should have expected update --no-save --no-package-lock lockfile result'
+  )
+})
+
+t.test('npm update --no-save', async t => {
+  const cmd = `${npmBin} update --no-save`
+  await exec(cmd)
+
+  t.equal(
+    JSON.parse(readFile('package.json')).dependencies.abbrev,
+    '^1.0.4',
+    'should have expected update --no-save package.json result'
+  )
+  t.equal(
+    JSON.parse(readFile('package-lock.json')).packages['node_modules/abbrev'].version,
+    '1.1.1',
+    'should have expected update --no-save lockfile result'
+  )
+})
+
 t.test('npm update --save', async t => {
   const cmd = `${npmBin} update --save`
-  const cmdRes = await exec(cmd)
+  await exec(cmd)
 
-  t.matchSnapshot(cmdRes.replace(/in.*s/, ''),
-    'should have expected update --save reify output')
-  t.matchSnapshot(readFile('package.json'),
-    'should have expected update --save package.json result')
-  t.matchSnapshot(readFile('package-lock.json'),
-    'should have expected update --save lockfile result')
+  t.equal(
+    JSON.parse(readFile('package.json')).dependencies.abbrev,
+    '^1.1.1',
+    'should have expected update --save package.json result'
+  )
+  t.equal(
+    JSON.parse(readFile('package-lock.json')).packages['node_modules/abbrev'].version,
+    '1.1.1',
+    'should have expected update --save lockfile result'
+  )
 })

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

@@ -666,64 +666,6 @@ removed 1 package
 
 `
 
-exports[`smoke-tests/index.js TAP npm update --save > should have expected update --save lockfile result 1`] = `
-{
-  "name": "project",
-  "version": "1.0.0",
-  "lockfileVersion": 2,
-  "requires": true,
-  "packages": {
-    "": {
-      "name": "project",
-      "version": "1.0.0",
-      "license": "ISC",
-      "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[`smoke-tests/index.js TAP npm update --save > should have expected update --save package.json result 1`] = `
-{
-  "name": "project",
-  "version": "1.0.0",
-  "description": "",
-  "main": "index.js",
-  "scripts": {
-    "test": "echo /"Error: no test specified/" && exit 1",
-    "hello": "echo Hello"
-  },
-  "keywords": [],
-  "author": "",
-  "license": "ISC",
-  "dependencies": {
-    "abbrev": "^1.1.1"
-  }
-}
-
-`
-
-exports[`smoke-tests/index.js TAP npm update --save > should have expected update --save reify output 1`] = `
-
-up to date 
-
-`
-
 exports[`smoke-tests/index.js TAP npm update dep > should have expected update lockfile result 1`] = `
 {
   "name": "project",

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

@@ -1406,16 +1406,16 @@ The base URL of the npm registry.
 exports[`test/lib/utils/config/definitions.js TAP > config description for save 1`] = `
 #### \`save\`
 
-* Default: true unless when using \`npm update\` or \`npm dedupe\` where it
+* Default: \`true\` unless when using \`npm update\` or \`npm dedupe\` where it
   defaults to \`false\`
 * Type: Boolean
 
-Save installed packages to a package.json file as dependencies.
+Save installed packages to a \`package.json\` file as dependencies.
 
 When used with the \`npm rm\` command, removes the dependency from
-package.json.
+\`package.json\`.
 
-Will also prevent writing to package-lock.json if set to \`false\`.
+Will also prevent writing to \`package-lock.json\` if set to \`false\`.
 `
 
 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

@@ -1200,16 +1200,16 @@ The base URL of the npm registry.
 
 #### \`save\`
 
-* Default: true unless when using \`npm update\` or \`npm dedupe\` where it
+* Default: \`true\` unless when using \`npm update\` or \`npm dedupe\` where it
   defaults to \`false\`
 * Type: Boolean
 
-Save installed packages to a package.json file as dependencies.
+Save installed packages to a \`package.json\` file as dependencies.
 
 When used with the \`npm rm\` command, removes the dependency from
-package.json.
+\`package.json\`.
 
-Will also prevent writing to package-lock.json if set to \`false\`.
+Will also prevent writing to \`package-lock.json\` if set to \`false\`.
 
 <!-- automatically generated, do not edit manually -->
 <!-- see lib/utils/config/definitions.js -->

workspaces/arborist/lib/arborist/reify.js

@@ -1142,21 +1142,25 @@ module.exports = cls => class Reifier extends cls {
     // for install failures.  Those still end up in the shrinkwrap, so we
     // save it first, then prune out the optional trash, and then return it.
 
-    const skipSave = options.save === false
-    const save = !skipSave
+    const save = !(options.save === false)
+
+    // we check for updates in order to make sure we run save ideal tree
+    // even though save=false since we want `npm update` to be able to
+    // write to package-lock files by default
     const hasUpdates = this[_updateAll] || this[_updateNames].length
 
     // we're going to completely skip save ideal tree in case of a global or
     // dry-run install and also if the save option is set to false, EXCEPT for
     // update since the expected behavior for npm7+ is for update to
     // NOT save to package.json, we make that exception since we still want
-    // saveIdealTree to be able to write the lockfile
-    const skipSaveIdealTree =
-      (skipSave && !hasUpdates)
+    // saveIdealTree to be able to write the lockfile by default.
+    const saveIdealTree = !(
+      (!save && !hasUpdates)
       || this[_global]
       || this[_dryRun]
+    )
 
-    if (skipSaveIdealTree) {
+    if (!saveIdealTree) {
       return false
     }