fix: remove dedupe --save

* Removed dedupe --save documentation and attempted implementation.
* Remove some unneeded otplease mocks from test

`npm dedupe --save` didn't work in a easy to understand way. It would
only update a top level dependency that was duplicated in the tree.
Found this out rewriting the dedupe tests to be real.  This is not very
intuitive and it's best if folks use update or install for saving to
package.json.

Author: wraithgar

docs/content/commands/npm-dedupe.md

@@ -80,11 +80,9 @@ 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).
+Note: `npm dedupe` will never update the semver values of direct
+dependencies in your project `package.json`, if you want to update
+values in `package.json` you can run: `npm update --save` instead.
 
 ### Configuration
 
@@ -158,22 +156,6 @@ This configuration does not affect `npm ci`.
 <!-- automatically generated, do not edit manually -->
 <!-- see lib/utils/config/definitions.js -->
 
-#### `save`
-
-* 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 -->
-
 #### `omit`
 
 * Default: 'dev' if the `NODE_ENV` environment variable is set to

lib/commands/dedupe.js

@@ -12,7 +12,6 @@ class Dedupe extends ArboristWorkspaceCmd {
     'legacy-bundling',
     'strict-peer-deps',
     'package-lock',
-    'save',
     'omit',
     'ignore-scripts',
     'audit',
@@ -29,19 +28,17 @@ 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,
       path: where,
       dryRun,
-      save,
+      // Saving during dedupe would only update if one of your direct
+      // dependencies was also duplicated somewhere in your tree. It would be
+      // confusing if running this were to also update your package.json.  In
+      // order to reduce potential confusion we set this to false.
+      save: false,
       workspaces: this.workspaceNames,
     }
     const arb = new Arborist(opts)

lib/npm.js

@@ -131,13 +131,16 @@ class Npm extends EventEmitter {
 
     const isGlobal = this.config.get('global')
     const workspacesEnabled = this.config.get('workspaces')
+    // if cwd is a workspace, the default is set to [that workspace]
     const implicitWorkspace = this.config.get('workspace', 'default').length > 0
     const workspacesFilters = this.config.get('workspace')
     const includeWorkspaceRoot = this.config.get('include-workspace-root')
     // only call execWorkspaces when we have workspaces explicitly set
     // or when it is implicit and not in our ignore list
     const hasWorkspaceFilters = workspacesFilters.length > 0
     const invalidWorkspaceConfig = workspacesEnabled === false && hasWorkspaceFilters
+
+    // (-ws || -w foo) && (cwd is not a workspace || command is not ignoring implicit workspaces)
     const filterByWorkspaces = (workspacesEnabled || hasWorkspaceFilters) &&
       (!implicitWorkspace || !command.ignoreImplicitWorkspace)
     // normally this would go in the constructor, but our tests don't

tap-snapshots/test/lib/load-all-commands.js.test.cjs

@@ -161,7 +161,6 @@ 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|--save-bundle]
 [--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> ...]]

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

@@ -293,7 +293,6 @@ 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|--save-bundle]
                     [--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> ...]]

test/fixtures/mock-npm.js

@@ -83,6 +83,10 @@ const LoadMockNpm = async (t, {
 
   const { Npm, ...rest } = RealMockNpm(t, mocks)
 
+  // We want to fail fast when writing tests. Default this to 0 unless it was
+  // explicitly set in a test.
+  config = { 'fetch-retries': 0, ...config }
+
   if (!init && load) {
     throw new Error('cant `load` without `init`')
   }

test/fixtures/mock-registry.js

@@ -0,0 +1,113 @@
+/*
+ * Mock registry class
+ *
+ * This should end up as the centralized place where we generate test fixtures
+ * for tests against any registry data.
+ */
+const pacote = require('pacote')
+class MockRegistry {
+  #tap
+  #nock
+  #registry
+  #authorization
+
+  constructor (opts) {
+    if (!opts.registry) {
+      throw new Error('mock registry requires a registry value')
+    }
+    this.#registry = (new URL(opts.registry)).origin
+    this.#authorization = opts.authorization
+    // Required for this.package
+    this.#tap = opts.tap
+  }
+
+  get nock () {
+    if (!this.#nock) {
+      const tnock = require('./tnock.js')
+      const reqheaders = {}
+      if (this.#authorization) {
+        reqheaders.authorization = `Bearer ${this.#authorization}`
+      }
+      this.#nock = tnock(this.#tap, this.#registry, { reqheaders })
+    }
+    return this.#nock
+  }
+
+  set nock (nock) {
+    this.#nock = nock
+  }
+
+  async package ({ manifest, times = 1, query, tarballs }) {
+    let nock = this.nock
+    if (!nock) {
+      throw new Error('cannot mock packages without a tap fixture')
+    }
+    nock = nock.get(`/${manifest.name}`).times(times)
+    if (query) {
+      nock = nock.query(query)
+    }
+    nock = nock.reply(200, manifest)
+    if (tarballs) {
+      for (const version in manifest.versions) {
+        const packument = manifest.versions[version]
+        const dist = new URL(packument.dist.tarball)
+        const tarball = await pacote.tarball(tarballs[version])
+        nock.get(dist.pathname).reply(200, tarball)
+      }
+    }
+    this.nock = nock
+  }
+
+  // the last packument in the packuments array will be tagged as latest
+  manifest ({ name = 'test-package', packuments } = {}) {
+    packuments = this.packuments(packuments, name)
+    const latest = packuments.slice(-1)[0]
+    const manifest = {
+      _id: `${name}@${latest.version}`,
+      _rev: '00-testdeadbeef',
+      name,
+      description: 'test package mock manifest',
+      dependencies: {},
+      versions: {},
+      time: {},
+      'dist-tags': { latest: latest.version },
+      ...latest,
+    }
+
+    for (const packument of packuments) {
+      manifest.versions[packument.version] = {
+        _id: `${name}@${packument.version}`,
+        name,
+        description: 'test package mock manifest',
+        dependencies: {},
+        dist: {
+          tarball: `${this.#registry}/${name}/-/${name}-${packument.version}.tgz`,
+        },
+        ...packument,
+      }
+      manifest.time[packument.version] = new Date()
+    }
+
+    return manifest
+  }
+
+  packuments (packuments = ['1.0.0'], name) {
+    return packuments.map(p => this.packument(p, name))
+  }
+
+  // Generate packument from shorthand
+  packument (packument, name = 'test-package') {
+    if (!packument.version) {
+      packument = { version: packument }
+    }
+    return {
+      name,
+      version: '1.0.0',
+      description: 'mocked test package',
+      dependencies: {},
+      ...packument,
+    }
+  }
+}
+
+module.exports = MockRegistry

test/lib/commands/dedupe.js

@@ -1,5 +1,9 @@
 const t = require('tap')
 const { load: loadMockNpm } = require('../../fixtures/mock-npm')
+const path = require('path')
+const fs = require('fs')
+
+const MockRegistry = require('../../fixtures/mock-registry.js')
 
 t.test('should throw in global mode', async (t) => {
   const { npm } = await loadMockNpm(t, {
@@ -14,45 +18,78 @@ t.test('should throw in global mode', async (t) => {
   )
 })
 
-t.test('should remove dupes using Arborist', async (t) => {
-  t.plan(5)
-  const { npm } = await loadMockNpm(t, {
-    mocks: {
-      '@npmcli/arborist': function (args) {
-        t.ok(args, 'gets options object')
-        t.ok(args.path, 'gets path option')
-        t.ok(args.dryRun, 'gets dryRun from user')
-        this.dedupe = () => {
-          t.ok(true, 'dedupe is called')
-        }
-      },
-      '../../lib/utils/reify-finish.js': (npm, arb) => {
-        t.ok(arb, 'gets arborist tree')
+const testTop = {
+  name: 'test-top',
+  version: '1.0.0',
+  dependencies: {
+    'test-dep-a': '*',
+    'test-dep-b': '*',
+  },
+}
+const testDepA = {
+  name: 'test-dep-a',
+  version: '1.0.1',
+  dependencies: { 'test-sub': '*' },
+}
+const testDepB = {
+  name: 'test-dep-b',
+  version: '1.0.0',
+  dependencies: { 'test-sub': '*' },
+}
+const testSub = {
+  name: 'test-sub',
+  version: '1.0.0',
+}
+
+const treeWithDupes = {
+  'package.json': JSON.stringify(testTop),
+  node_modules: {
+    'test-dep-a': {
+      'package.json': JSON.stringify(testDepA),
+      node_modules: {
+        'test-sub': {
+          'package.json': JSON.stringify(testSub),
+        },
       },
     },
-    config: {
-      'dry-run': 'true',
+    'test-dep-b': {
+      'package.json': JSON.stringify(testDepB),
+      node_modules: {
+        'test-sub': {
+          'package.json': JSON.stringify(testSub),
+        },
+      },
     },
+  },
+}
+
+t.test('dedupe', async (t) => {
+  const { npm, joinedOutput } = await loadMockNpm(t, {
+    prefixDir: treeWithDupes,
+  })
+  const registry = new MockRegistry({
+    tap: t,
+    registry: npm.config.get('registry'),
+  })
+  const manifestSub = registry.manifest({
+    name: 'test-sub',
+    packuments: [{ version: '1.0.0' }],
   })
-  await npm.exec('dedupe', [])
-})
 
-t.test('should remove dupes using Arborist - no arguments', async (t) => {
-  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 registry.package({
+    manifest: manifestSub,
+    tarballs: {
+      '1.0.0': path.join(npm.prefix, 'node_modules', 'test-dep-a', 'node_modules', 'test-sub'),
     },
   })
   await npm.exec('dedupe', [])
+  t.match(joinedOutput(), /added 1 package, and removed 2 packages/)
+  t.ok(fs.existsSync(path.join(npm.prefix, 'node_modules', 'test-sub')), 'test-sub was hoisted')
+  t.notOk(
+    fs.existsSync(path.join(npm.prefix, 'node_modules', 'test-dep-a', 'node_modules', 'test-sub')),
+    'test-dep-a/test-sub was removed'
+  )
+  t.notOk(
+    fs.existsSync(path.join(npm.prefix, 'node_modules', 'test-dep-b', 'node_modules', 'test-sub')),
+    'test-dep-b/test-sub was removed')
 })