Skip to content

Commit

Permalink
fix: remove dedupe --save
Browse files Browse the repository at this point in the history
* 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.
  • Loading branch information
wraithgar committed Mar 30, 2022
1 parent d088c4c commit aba3f33
Show file tree
Hide file tree
Showing 9 changed files with 389 additions and 175 deletions.
8 changes: 3 additions & 5 deletions docs/content/commands/npm-dedupe.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
13 changes: 5 additions & 8 deletions lib/commands/dedupe.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ class Dedupe extends ArboristWorkspaceCmd {
'legacy-bundling',
'strict-peer-deps',
'package-lock',
'save',
'omit',
'ignore-scripts',
'audit',
Expand All @@ -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)
Expand Down
1 change: 0 additions & 1 deletion tap-snapshots/test/lib/load-all-commands.js.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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> ...]]
Expand Down
1 change: 0 additions & 1 deletion tap-snapshots/test/lib/utils/npm-usage.js.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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> ...]]
Expand Down
39 changes: 39 additions & 0 deletions test/fixtures/mock-manifest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@

// Will tag the last entry in versions as latest
// Versions can be an array of versions or an array of package.json
function mockManifest ({ name = 'test-package', versions = ['1.0.0']}) {
versions = versions.map(v => {
if (!v.version) {
return { version: v }
}
return v
})
const latest = versions.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 v of versions) {
manifest.versions[v.version] = {
_id: `${name}@{v.version}`,
name,
description: 'test package mock manifest',
dependencies: {},
dist: {
tarball: `https://registry.npmjs.org/${name}/-/${name}-${v.version}.tgz`,
},
...v,
}
manifest.time[v.version] = new Date()
}

return manifest
}
module.exports = mockManifest
4 changes: 4 additions & 0 deletions test/fixtures/mock-npm.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`')
}
Expand Down
120 changes: 120 additions & 0 deletions test/fixtures/mock-registry.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
/*
* 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
}

//tar contents are all under package/
// so package/package.json

tarball({ cwd, files }) {
const tarball = pacote.tarball(cwd)
}
// 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
97 changes: 62 additions & 35 deletions test/lib/commands/dedupe.js
Original file line number Diff line number Diff line change
@@ -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, {
Expand All @@ -14,45 +18,68 @@ 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
})
await npm.exec('dedupe', [])
})
const registry = new MockRegistry({
tap: t,
registry: npm.config.get('registry'),
})
const manifestSub = registry.manifest({ name: testSub.name, manifests: [testSub] })

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

0 comments on commit aba3f33

Please sign in to comment.