fix: npm update --save (#4223)

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.

- 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

Fixes: #708
Fixes: #2704
Relates to: npm/feedback#270

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,13 +1326,16 @@ 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.
+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`.
 
 <!-- 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',
@@ -40,19 +41,27 @@ class Update extends ArboristWorkspaceCmd {
       ? global
       : this.npm.prefix
 
+    // In the context of `npm update` the save
+    // config value should default to `false`
+    const save = this.npm.config.isDefault('save')
+      ? false
+      : this.npm.config.get('save')
+
     if (this.npm.config.get('depth')) {
       log.warn('update', 'The --depth option no longer has any effect. See RFC0019.\n' +
         'https://github.com/npm/rfcs/blob/latest/implemented/0019-remove-update-depth-option.md')
     }
 
-    const arb = new Arborist({
+    const opts = {
       ...this.npm.flatOptions,
       log,
       path: where,
+      save,
       workspaces: this.workspaceNames,
-    })
+    }
+    const arb = new Arborist(opts)
 
-    await arb.reify({ update })
+    await arb.reify({ ...opts, update })
     await reifyFinish(this.npm, arb)
   }
 }

lib/utils/config/definitions.js

@@ -1583,14 +1583,18 @@ 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',
   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\`.
   `,
   flatten,
 })

smoke-tests/index.js

@@ -267,3 +267,56 @@ t.test('npm pkg', async t => {
     'should have expected npm pkg delete modified package.json result'
   )
 })
+
+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`
+  await exec(cmd)
+
+  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/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.
+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\`.
 `
 
 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,13 +1200,16 @@ 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.
+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\`.
 
 <!-- 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/fixtures/mock-npm.js

@@ -147,6 +147,8 @@ class MockNpm {
       // for now just set `find` to what config.find should return
       // this works cause `find` is not an existing config entry
       find: (k) => ({ ...realConfig.defaults, ...config })[k],
+      // for now isDefault is going to just return false if a value was defined
+      isDefault: (k) => !Object.prototype.hasOwnProperty.call(config, k),
       get: (k) => ({ ...realConfig.defaults, ...config })[k],
       set: (k, v) => config[k] = v,
       list: [{ ...realConfig.defaults, ...config }],

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', [])

test/lib/commands/update.js

@@ -27,7 +27,7 @@ t.afterEach(() => {
 })
 
 t.test('no args', async t => {
-  t.plan(4)
+  t.plan(5)
 
   npm.prefix = '/project/a'
 
@@ -39,14 +39,16 @@ t.test('no args', async t => {
         {
           ...npm.flatOptions,
           path: npm.prefix,
+          save: false,
           workspaces: null,
         },
         'should call arborist contructor with expected args'
       )
       t.match(log, {}, 'log is passed in')
     }
 
-    reify ({ update }) {
+    reify ({ save, update }) {
+      t.equal(save, false, 'should default to save=false')
       t.equal(update, true, 'should update all deps')
     }
   }
@@ -64,9 +66,10 @@ t.test('no args', async t => {
 })
 
 t.test('with args', async t => {
-  t.plan(4)
+  t.plan(5)
 
   npm.prefix = '/project/a'
+  config.save = true
 
   class Arborist {
     constructor (args) {
@@ -76,14 +79,16 @@ t.test('with args', async t => {
         {
           ...npm.flatOptions,
           path: npm.prefix,
+          save: true,
           workspaces: null,
         },
         'should call arborist contructor with expected args'
       )
       t.match(log, {}, 'log is passed in')
     }
 
-    reify ({ update }) {
+    reify ({ save, update }) {
+      t.equal(save, true, 'should pass save if manually set')
       t.same(update, ['ipt'], 'should update listed deps')
     }
   }
@@ -140,7 +145,7 @@ t.test('update --global', async t => {
       const { path, log, ...rest } = args
       t.same(
         rest,
-        { ...npm.flatOptions, workspaces: undefined },
+        { ...npm.flatOptions, save: true, workspaces: undefined },
         'should call arborist contructor with expected options'
       )
 

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')