fix: default save config value for update cmd

- Uses config.isDefault to set a different value for the `save` config
  for both the update and dedupe commands
- Tweaks arborist to make sure saveIdealTree preserves the behavior of
  skipping writing to package-lock.json on save=false for install while
  still writing the lockfile for `npm update` with its new default value
  of save=false.
- Updated and added some new tests on arborist to cover for these tweaks
- Added `npm update --save` smoke test on cli

docs/content/commands/npm-dedupe.md

@@ -72,6 +72,12 @@ result in new modules being installed.
 
 Using `npm find-dupes` will run the command in `--dry-run` mode.
 
+Note that by default `npm dedupe` will not update the semver values of direct
+dependencies in your project `package.json`, if you want to also update
+values in `package.json` you can run: `npm dedupe --save` (or add the
+`save=true` option to a [configuration file](/configuring-npm/npmrc)
+to make that the default behavior).
+
 ### Configuration
 
 <!-- AUTOGENERATED CONFIG DESCRIPTIONS START -->

docs/content/commands/npm-update.md

@@ -27,6 +27,12 @@ packages.
 If no package name is specified, all packages in the specified location (global
 or local) will be updated.
 
+Note that by default `npm update` will not update the semver values of direct
+dependencies in your project `package.json`, if you want to also update
+values in `package.json` you can run: `npm update --save` (or add the
+`save=true` option to a [configuration file](/configuring-npm/npmrc)
+to make that the default behavior).
+
 ### Example
 
 For the examples below, assume that the current package is `app` and it depends

docs/content/using-npm/config.md

@@ -1326,14 +1326,17 @@ The base URL of the npm registry.
 
 #### `save`
 
-* Default: true
+* 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.
 
 When used with the `npm rm` command, removes the dependency from
 package.json.
 
+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/dedupe.js

@@ -13,6 +13,7 @@ class Dedupe extends ArboristWorkspaceCmd {
     'legacy-bundling',
     'strict-peer-deps',
     'package-lock',
+    'save',
     'omit',
     'ignore-scripts',
     'audit',
@@ -29,13 +30,20 @@ class Dedupe extends ArboristWorkspaceCmd {
       throw er
     }
 
+    // In the context of `npm dedupe` the save
+    // config value should default to `false`
+    const save = this.npm.config.isDefault('save')
+      ? false
+      : this.npm.config.get('save')
+
     const dryRun = this.npm.config.get('dry-run')
     const where = this.npm.prefix
     const opts = {
       ...this.npm.flatOptions,
       log,
       path: where,
       dryRun,
+      save,
       workspaces: this.workspaceNames,
     }
     const arb = new Arborist(opts)

lib/commands/update.js

@@ -16,6 +16,7 @@ class Update extends ArboristWorkspaceCmd {
     'legacy-bundling',
     'strict-peer-deps',
     'package-lock',
+    'save',
     'omit',
     'ignore-scripts',
     'audit',

lib/utils/config/definitions.js

@@ -1583,6 +1583,8 @@ define('registry', {
 
 define('save', {
   default: true,
+  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',
@@ -1591,6 +1593,8 @@ define('save', {
 
     When used with the \`npm rm\` command, removes the dependency from
     package.json.
+
+    Will also prevent writing to package-lock.json if set to \`false\`.
   `,
   flatten,
 })

smoke-tests/index.js

@@ -267,3 +267,13 @@ t.test('npm pkg', async t => {
     'should have expected npm pkg delete modified package.json result'
   )
 })
+
+t.test('npm update --save', async t => {
+  const cmd = `${npmBin} update --save`
+  const cmdRes = 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')
+})
+

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

@@ -666,6 +666,64 @@ 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/load-all-commands.js.test.cjs

@@ -170,6 +170,7 @@ npm dedupe
 
 Options:
 [--global-style] [--legacy-bundling] [--strict-peer-deps] [--no-package-lock]
+[-S|--save|--no-save|--save-prod|--save-dev|--save-optional|--save-peer]
 [--omit <dev|optional|peer> [--omit <dev|optional|peer> ...]] [--ignore-scripts]
 [--no-audit] [--no-bin-links] [--no-fund] [--dry-run]
 [-w|--workspace <workspace-name> [-w|--workspace <workspace-name> ...]]
@@ -1061,8 +1062,10 @@ npm update [<pkg>...]
 
 Options:
 [-g|--global] [--global-style] [--legacy-bundling] [--strict-peer-deps]
-[--no-package-lock] [--omit <dev|optional|peer> [--omit <dev|optional|peer> ...]]
-[--ignore-scripts] [--no-audit] [--no-bin-links] [--no-fund] [--dry-run]
+[--no-package-lock]
+[-S|--save|--no-save|--save-prod|--save-dev|--save-optional|--save-peer]
+[--omit <dev|optional|peer> [--omit <dev|optional|peer> ...]] [--ignore-scripts]
+[--no-audit] [--no-bin-links] [--no-fund] [--dry-run]
 [-w|--workspace <workspace-name> [-w|--workspace <workspace-name> ...]]
 [-ws|--workspaces] [--include-workspace-root]
 

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

@@ -1406,13 +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
+* 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.
 
 When used with the \`npm rm\` command, removes the dependency from
 package.json.
+
+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,14 +1200,17 @@ The base URL of the npm registry.
 
 #### \`save\`
 
-* Default: true
+* 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.
 
 When used with the \`npm rm\` command, removes the dependency from
 package.json.
 
+Will also prevent writing to package-lock.json if set to \`false\`.
+
 <!-- automatically generated, do not edit manually -->
 <!-- see lib/utils/config/definitions.js -->
 

tap-snapshots/test/lib/utils/npm-usage.js.test.cjs

@@ -311,6 +311,7 @@ All commands:
                     
                     Options:
                     [--global-style] [--legacy-bundling] [--strict-peer-deps] [--no-package-lock]
+                    [-S|--save|--no-save|--save-prod|--save-dev|--save-optional|--save-peer]
                     [--omit <dev|optional|peer> [--omit <dev|optional|peer> ...]] [--ignore-scripts]
                     [--no-audit] [--no-bin-links] [--no-fund] [--dry-run]
                     [-w|--workspace <workspace-name> [-w|--workspace <workspace-name> ...]]
@@ -1096,8 +1097,10 @@ All commands:
                     
                     Options:
                     [-g|--global] [--global-style] [--legacy-bundling] [--strict-peer-deps]
-                    [--no-package-lock] [--omit <dev|optional|peer> [--omit <dev|optional|peer> ...]]
-                    [--ignore-scripts] [--no-audit] [--no-bin-links] [--no-fund] [--dry-run]
+                    [--no-package-lock]
+                    [-S|--save|--no-save|--save-prod|--save-dev|--save-optional|--save-peer]
+                    [--omit <dev|optional|peer> [--omit <dev|optional|peer> ...]] [--ignore-scripts]
+                    [--no-audit] [--no-bin-links] [--no-fund] [--dry-run]
                     [-w|--workspace <workspace-name> [-w|--workspace <workspace-name> ...]]
                     [-ws|--workspaces] [--include-workspace-root]
                     

test/lib/commands/dedupe.js

@@ -38,18 +38,20 @@ t.test('should remove dupes using Arborist', async (t) => {
 })
 
 t.test('should remove dupes using Arborist - no arguments', async (t) => {
-  t.plan(1)
+  t.plan(2)
   const { npm } = await loadMockNpm(t, {
     mocks: {
       '@npmcli/arborist': function (args) {
         t.ok(args.dryRun, 'gets dryRun from config')
+        t.ok(args.save, 'gets user-set save value from config')
         this.dedupe = () => {}
       },
       '../../lib/utils/reify-output.js': () => {},
       '../../lib/utils/reify-finish.js': () => {},
     },
     config: {
       'dry-run': true,
+      save: true,
     },
   })
   await npm.exec('dedupe', [])

workspaces/arborist/lib/arborist/reify.js

@@ -1142,8 +1142,21 @@ 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.
 
-    // support save=false option
-    if (options.save === false || this[_global] || this[_dryRun]) {
+    const skipSave = options.save === false
+    const save = !skipSave
+    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)
+      || this[_global]
+      || this[_dryRun]
+
+    if (skipSaveIdealTree) {
       return false
     }
 
@@ -1165,7 +1178,7 @@ module.exports = cls => class Reifier extends cls {
         // that we couldn't resolve, this MAY be missing.  if we haven't
         // blown up by now, it's because it was not a problem, though, so
         // just move on.
-        if (!child) {
+        if (!child || !addTree.isTop) {
           continue
         }
 
@@ -1284,25 +1297,32 @@ module.exports = cls => class Reifier extends cls {
       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])
+    if (save) {
+      // when using update all alongside with save, we'll make
+      // sure to refresh every dependency of the root idealTree
+      if (this[_updateAll]) {
+        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) {
+          const nodes = retrieveUpdatedNodes(this[_updateNames])
+          updateNodes(nodes)
+        }
+
+        // grab any from explicitRequests that had deps removed
+        for (const { from: tree } of this.explicitRequests) {
+          updatedTrees.add(tree)
+        }
       }
     }
 
@@ -1338,15 +1358,12 @@ module.exports = cls => class Reifier extends cls {
       await pkgJson.save()
     }
 
-    // grab any from explicitRequests that had deps removed
-    for (const { from: tree } of this.explicitRequests) {
-      updatedTrees.add(tree)
-    }
-
-    for (const tree of updatedTrees) {
-      // refresh the edges so they have the correct specs
-      tree.package = tree.package
-      promises.push(updatePackageJson(tree))
+    if (save) {
+      for (const tree of updatedTrees) {
+        // refresh the edges so they have the correct specs
+        tree.package = tree.package
+        promises.push(updatePackageJson(tree))
+      }
     }
 
     await Promise.all(promises)