@@ -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

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

@@ -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

@@ -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> ...]]

@@ -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> ...]]

@@ -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`')
 }

@@ -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

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