From fde35466915b5ac5958c827fa7e919e1f186db51 Mon Sep 17 00:00:00 2001 From: Gar Date: Fri, 14 May 2021 07:00:06 -0700 Subject: [PATCH] feat(unpublish): add workspace/dry-run support PR-URL: https://github.com/npm/cli/pull/3251 Credit: @wraithgar Close: #3251 Reviewed-by: @ruyadorno, @isaacs --- docs/content/commands/npm-unpublish.md | 45 ++++ lib/publish.js | 2 +- lib/unpublish.js | 58 +++-- .../test/lib/load-all-commands.js.test.cjs | 4 +- tap-snapshots/test/lib/publish.js.test.cjs | 23 +- tap-snapshots/test/lib/unpublish.js.test.cjs | 14 ++ .../test/lib/utils/npm-usage.js.test.cjs | 4 +- test/lib/publish.js | 8 +- test/lib/unpublish.js | 209 ++++++++++++------ 9 files changed, 269 insertions(+), 98 deletions(-) create mode 100644 tap-snapshots/test/lib/unpublish.js.test.cjs diff --git a/docs/content/commands/npm-unpublish.md b/docs/content/commands/npm-unpublish.md index 3f7b31b4ee8f1..bc8fbc7a53b3d 100644 --- a/docs/content/commands/npm-unpublish.md +++ b/docs/content/commands/npm-unpublish.md @@ -49,6 +49,19 @@ passed. +#### `dry-run` + +* Default: false +* Type: Boolean + +Indicates that you don't want npm to make any changes and that it should +only report what it would have done. This can be passed into any of the +commands that modify your local installation, eg, `install`, `update`, +`dedupe`, `uninstall`, as well as `pack` and `publish`. + +Note: This is NOT honored by other network related commands, eg `dist-tags`, +`owner`, etc. + #### `force` * Default: false @@ -73,6 +86,38 @@ mistakes, unnecessary performance degradation, and malicious input. If you don't have a clear idea of what you want to do, it is strongly recommended that you do not use this option! +#### `workspace` + +* Default: +* Type: String (can be set multiple times) + +Enable running a command in the context of the configured workspaces of the +current project while filtering by running only the workspaces defined by +this configuration option. + +Valid values for the `workspace` config are either: + +* Workspace names +* Path to a workspace directory +* Path to a parent workspace directory (will result to selecting all of the + nested workspaces) + +When set for the `npm init` command, this may be set to the folder of a +workspace which does not yet exist, to create the folder and set it up as a +brand new workspace within the project. + +This value is not exported to the environment for child processes. + +#### `workspaces` + +* Default: false +* Type: Boolean + +Enable running a command in the context of **all** the configured +workspaces. + +This value is not exported to the environment for child processes. + ### See Also diff --git a/lib/publish.js b/lib/publish.js index ae371b9727791..1693ea7d97c2a 100644 --- a/lib/publish.js +++ b/lib/publish.js @@ -58,7 +58,7 @@ class Publish extends BaseCommand { if (args.length === 0) args = ['.'] if (args.length !== 1) - throw this.usage + throw this.usageError() log.verbose('publish', args) diff --git a/lib/unpublish.js b/lib/unpublish.js index 8030bb403fbb3..1571fd88ef781 100644 --- a/lib/unpublish.js +++ b/lib/unpublish.js @@ -1,12 +1,12 @@ const path = require('path') const util = require('util') -const log = require('npmlog') const npa = require('npm-package-arg') const libaccess = require('libnpmaccess') const npmFetch = require('npm-registry-fetch') const libunpub = require('libnpmpublish').unpublish const readJson = util.promisify(require('read-package-json')) +const getWorkspaces = require('./workspaces/get-workspaces.js') const otplease = require('./utils/otplease.js') const getIdentity = require('./utils/get-identity.js') @@ -22,13 +22,13 @@ class Unpublish extends BaseCommand { } /* istanbul ignore next - see test/lib/load-all-commands.js */ - static get usage () { - return ['[<@scope>/][@]'] + static get params () { + return ['dry-run', 'force', 'workspace', 'workspaces'] } /* istanbul ignore next - see test/lib/load-all-commands.js */ - static get params () { - return ['force'] + static get usage () { + return ['[<@scope>/][@]'] } async completion (args) { @@ -67,25 +67,29 @@ class Unpublish extends BaseCommand { this.unpublish(args).then(() => cb()).catch(cb) } + execWorkspaces (args, filters, cb) { + this.unpublishWorkspaces(args, filters).then(() => cb()).catch(cb) + } + async unpublish (args) { if (args.length > 1) - throw new Error(this.usage) + throw this.usageError() const spec = args.length && npa(args[0]) const force = this.npm.config.get('force') - const silent = this.npm.config.get('silent') const loglevel = this.npm.config.get('loglevel') + const silent = loglevel === 'silent' + const dryRun = this.npm.config.get('dry-run') let pkgName let pkgVersion - log.silly('unpublish', 'args[0]', args[0]) - log.silly('unpublish', 'spec', spec) + this.npm.log.silly('unpublish', 'args[0]', args[0]) + this.npm.log.silly('unpublish', 'spec', spec) - if (!spec.rawSpec && !force) { - throw new Error( + if ((!spec || !spec.rawSpec) && !force) { + throw this.usageError( 'Refusing to delete entire project.\n' + - 'Run with --force to do this.\n' + - this.usage + 'Run with --force to do this.' ) } @@ -101,25 +105,43 @@ class Unpublish extends BaseCommand { if (err && err.code !== 'ENOENT' && err.code !== 'ENOTDIR') throw err else - throw new Error(`Usage: ${this.usage}`) + throw this.usageError() } - log.verbose('unpublish', manifest) + this.npm.log.verbose('unpublish', manifest) const { name, version, publishConfig } = manifest const pkgJsonSpec = npa.resolve(name, version) const optsWithPub = { ...opts, publishConfig } - await otplease(opts, opts => libunpub(pkgJsonSpec, optsWithPub)) + if (!dryRun) + await otplease(opts, opts => libunpub(pkgJsonSpec, optsWithPub)) pkgName = name pkgVersion = version ? `@${version}` : '' } else { - await otplease(opts, opts => libunpub(spec, opts)) + if (!dryRun) + await otplease(opts, opts => libunpub(spec, opts)) pkgName = spec.name pkgVersion = spec.type === 'version' ? `@${spec.rawSpec}` : '' } - if (!silent && loglevel !== 'silent') + if (!silent) this.npm.output(`- ${pkgName}${pkgVersion}`) } + + async unpublishWorkspaces (args, filters) { + const workspaces = + await getWorkspaces(filters, { path: this.npm.localPrefix }) + + const force = this.npm.config.get('force') + if (!force) { + throw this.usageError( + 'Refusing to delete entire project(s).\n' + + 'Run with --force to do this.' + ) + } + + for (const [name] of workspaces.entries()) + await this.unpublish([name]) + } } module.exports = Unpublish diff --git a/tap-snapshots/test/lib/load-all-commands.js.test.cjs b/tap-snapshots/test/lib/load-all-commands.js.test.cjs index a1f4394528b43..e45239089da7e 100644 --- a/tap-snapshots/test/lib/load-all-commands.js.test.cjs +++ b/tap-snapshots/test/lib/load-all-commands.js.test.cjs @@ -1005,7 +1005,9 @@ Usage: npm unpublish [<@scope>/][@] Options: -[-f|--force] +[--dry-run] [-f|--force] +[-w|--workspace [-w|--workspace ...]] +[-ws|--workspaces] Run "npm help unpublish" for more info ` diff --git a/tap-snapshots/test/lib/publish.js.test.cjs b/tap-snapshots/test/lib/publish.js.test.cjs index 7d5e7a9c5ff88..05f0e6580ab5a 100644 --- a/tap-snapshots/test/lib/publish.js.test.cjs +++ b/tap-snapshots/test/lib/publish.js.test.cjs @@ -6,7 +6,8 @@ */ 'use strict' exports[`test/lib/publish.js TAP shows usage with wrong set of arguments > should print usage 1`] = ` -npm publish +Error: +Usage: npm publish Publish a package @@ -18,13 +19,16 @@ Options: [-w|--workspace [-w|--workspace ...]] [-ws|--workspaces] -Run "npm help publish" for more info +Run "npm help publish" for more info { + "code": "EUSAGE", +} ` exports[`test/lib/publish.js TAP workspaces all workspaces > should output all publishes 1`] = ` Array [ "+ workspace-a@1.2.3-a", "+ workspace-b@1.2.3-n", + "+ workspace-n@1.2.3-n", ] ` @@ -54,6 +58,12 @@ Array [ }, "version": "1.2.3-n", }, + Object { + "_id": "workspace-n@1.2.3-n", + "name": "workspace-n", + "readme": "ERROR: No README data found!", + "version": "1.2.3-n", + }, ] ` @@ -66,6 +76,9 @@ Array [ }, "workspace-b": { "id": "workspace-b@1.2.3-n" + }, + "workspace-n": { + "id": "workspace-n@1.2.3-n" } } ), @@ -98,6 +111,12 @@ Array [ }, "version": "1.2.3-n", }, + Object { + "_id": "workspace-n@1.2.3-n", + "name": "workspace-n", + "readme": "ERROR: No README data found!", + "version": "1.2.3-n", + }, ] ` diff --git a/tap-snapshots/test/lib/unpublish.js.test.cjs b/tap-snapshots/test/lib/unpublish.js.test.cjs new file mode 100644 index 0000000000000..5936bec6c759f --- /dev/null +++ b/tap-snapshots/test/lib/unpublish.js.test.cjs @@ -0,0 +1,14 @@ +/* IMPORTANT + * This snapshot file is auto-generated, but designed for humans. + * It should be checked into source control and tracked carefully. + * Re-generate by setting TAP_SNAPSHOT=1 and running tests. + * Make sure to inspect the output below. Do not ignore changes! + */ +'use strict' +exports[`test/lib/unpublish.js TAP workspaces all workspaces --force > should output all workspaces 1`] = ` +- workspace-a- workspace-b- workspace-n +` + +exports[`test/lib/unpublish.js TAP workspaces one workspace --force > should output one workspaces 1`] = ` +- workspace-a +` diff --git a/tap-snapshots/test/lib/utils/npm-usage.js.test.cjs b/tap-snapshots/test/lib/utils/npm-usage.js.test.cjs index 300faaa773b79..91ad8954d2296 100644 --- a/tap-snapshots/test/lib/utils/npm-usage.js.test.cjs +++ b/tap-snapshots/test/lib/utils/npm-usage.js.test.cjs @@ -1046,7 +1046,9 @@ All commands: npm unpublish [<@scope>/][@] Options: - [-f|--force] + [--dry-run] [-f|--force] + [-w|--workspace [-w|--workspace ...]] + [-ws|--workspaces] Run "npm help unpublish" for more info diff --git a/test/lib/publish.js b/test/lib/publish.js index 7aff7b08c1971..80f3f7ccc639e 100644 --- a/test/lib/publish.js +++ b/test/lib/publish.js @@ -538,12 +538,12 @@ t.test('workspaces', (t) => { repository: 'https://github.com/npm/workspace-b', }), }, - 'workspace-c': JSON.stringify({ - 'package.json': { + 'workspace-c': { + 'package.json': JSON.stringify({ name: 'workspace-n', version: '1.2.3-n', - }, - }), + }), + }, }) const publishes = [] diff --git a/test/lib/unpublish.js b/test/lib/unpublish.js index 9af7242f29787..6ad45e958c22f 100644 --- a/test/lib/unpublish.js +++ b/test/lib/unpublish.js @@ -5,41 +5,46 @@ let result = '' const noop = () => null const config = { force: false, - silent: false, loglevel: 'silly', } + +const testDir = t.testdir({ + 'package.json': JSON.stringify({ + name: 'pkg', + version: '1.0.0', + }, null, 2), +}) + const npm = mockNpm({ - localPrefix: '', + localPrefix: testDir, + log: { silly () {}, verbose () {} }, config, output: (...msg) => { result += msg.join('\n') }, }) + const mocks = { - npmlog: { silly () {}, verbose () {} }, libnpmaccess: { lsPackages: noop }, libnpmpublish: { unpublish: noop }, - 'npm-package-arg': noop, 'npm-registry-fetch': { json: noop }, - 'read-package-json': cb => cb(), '../../lib/utils/otplease.js': async (opts, fn) => fn(opts), - '../../lib/utils/usage.js': () => 'usage instructions', '../../lib/utils/get-identity.js': async () => 'foo', } t.afterEach(() => { + npm.log = { silly () {}, verbose () {} } + npm.localPrefix = testDir result = '' + config['dry-run'] = false config.force = false config.loglevel = 'silly' - config.silent = false }) t.test('no args --force', t => { - t.plan(9) - config.force = true - const npmlog = { + npm.log = { silly (title) { t.equal(title, 'unpublish', 'should silly log args') }, @@ -53,17 +58,9 @@ t.test('no args --force', t => { }, } - const npa = { - resolve (name, version) { - t.equal(name, 'pkg', 'should npa.resolve package name') - t.equal(version, '1.0.0', 'should npa.resolve package version') - return 'pkg@1.0.0' - }, - } - const libnpmpublish = { unpublish (spec, opts) { - t.equal(spec, 'pkg@1.0.0', 'should unpublish expected spec') + t.equal(spec.raw, 'pkg@1.0.0', 'should unpublish expected spec') t.same( opts, { @@ -76,14 +73,9 @@ t.test('no args --force', t => { const Unpublish = t.mock('../../lib/unpublish.js', { ...mocks, - npmlog, libnpmpublish, - 'npm-package-arg': npa, - 'read-package-json': (path, cb) => cb(null, { - name: 'pkg', - version: '1.0.0', - }), }) + const unpublish = new Unpublish(npm) unpublish.exec([], err => { @@ -95,25 +87,24 @@ t.test('no args --force', t => { '- pkg@1.0.0', 'should output removed pkg@version on success' ) + t.end() }) }) t.test('no args --force missing package.json', t => { config.force = true + const testDir = t.testdir({}) + npm.localPrefix = testDir const Unpublish = t.mock('../../lib/unpublish.js', { ...mocks, - 'read-package-json': (path, cb) => cb(Object.assign( - new Error('ENOENT'), - { code: 'ENOENT' } - )), }) const unpublish = new Unpublish(npm) unpublish.exec([], err => { t.match( err, - /usage instructions/, + /Usage: npm unpublish/, 'should throw usage instructions on missing package.json' ) t.end() @@ -164,7 +155,7 @@ t.test('too many args', t => { unpublish.exec(['a', 'b'], err => { t.match( err, - /usage instructions/, + /Usage: npm unpublish/, 'should throw usage instructions if too many args' ) t.end() @@ -172,29 +163,19 @@ t.test('too many args', t => { }) t.test('unpublish @version', t => { - t.plan(7) - - const pa = { - name: 'pkg', - rawSpec: '1.0.0', - type: 'version', - } - - const npmlog = { + npm.log = { silly (title, key, value) { t.equal(title, 'unpublish', 'should silly log args') if (key === 'spec') - t.equal(value, pa, 'should log parsed npa object') + t.match(value, { name: 'pkg', rawSpec: '1.0.0' }) else t.equal(value, 'pkg@1.0.0', 'should log originally passed arg') }, } - const npa = () => pa - const libnpmpublish = { unpublish (spec, opts) { - t.equal(spec, pa, 'should unpublish expected parsed spec') + t.equal(spec.raw, 'pkg@1.0.0', 'should unpublish expected parsed spec') t.same( opts, {}, @@ -205,9 +186,7 @@ t.test('unpublish @version', t => { const Unpublish = t.mock('../../lib/unpublish.js', { ...mocks, - npmlog, libnpmpublish, - 'npm-package-arg': npa, }) const unpublish = new Unpublish(npm) @@ -220,25 +199,22 @@ t.test('unpublish @version', t => { '- pkg@1.0.0', 'should output removed pkg@version on success' ) + t.end() }) }) t.test('no version found in package.json', t => { config.force = true - const npa = () => ({ - name: 'pkg', - type: 'version', + const testDir = t.testdir({ + 'package.json': JSON.stringify({ + name: 'pkg', + }, null, 2), }) - - npa.resolve = () => '' + npm.localPrefix = testDir const Unpublish = t.mock('../../lib/unpublish.js', { ...mocks, - 'npm-package-arg': npa, - 'read-package-json': (path, cb) => cb(null, { - name: 'pkg', - }), }) const unpublish = new Unpublish(npm) @@ -260,11 +236,6 @@ t.test('unpublish --force no version set', t => { const Unpublish = t.mock('../../lib/unpublish.js', { ...mocks, - 'npm-package-arg': () => ({ - name: 'pkg', - rawSpec: '', - type: 'tag', - }), }) const unpublish = new Unpublish(npm) @@ -284,28 +255,127 @@ t.test('unpublish --force no version set', t => { t.test('silent', t => { config.loglevel = 'silent' - const npa = () => ({ - name: 'pkg', - rawSpec: '1.0.0', - type: 'version', + const Unpublish = t.mock('../../lib/unpublish.js', { + ...mocks, }) + const unpublish = new Unpublish(npm) + + unpublish.exec(['pkg@1.0.0'], err => { + if (err) + throw err - npa.resolve = () => '' + t.equal( + result, + '', + 'should have no output' + ) + t.end() + }) +}) +t.test('workspaces', t => { + const testDir = t.testdir({ + 'package.json': JSON.stringify({ + name: 'my-cool-pkg', + version: '1.0.0', + workspaces: ['workspace-a', 'workspace-b', 'workspace-c'], + }, null, 2), + 'workspace-a': { + 'package.json': JSON.stringify({ + name: 'workspace-a', + version: '1.2.3-a', + repository: 'http://repo.workspace-a/', + }), + }, + 'workspace-b': { + 'package.json': JSON.stringify({ + name: 'workspace-b', + version: '1.2.3-n', + repository: 'https://github.com/npm/workspace-b', + }), + }, + 'workspace-c': { + 'package.json': JSON.stringify({ + name: 'workspace-n', + version: '1.2.3-n', + }), + }, + }) const Unpublish = t.mock('../../lib/unpublish.js', { ...mocks, - 'npm-package-arg': npa, }) const unpublish = new Unpublish(npm) + t.test('no force', (t) => { + npm.localPrefix = testDir + unpublish.execWorkspaces([], [], (err) => { + t.match(err, /--force/, 'should require force') + t.end() + }) + }) + + t.test('all workspaces --force', (t) => { + npm.localPrefix = testDir + config.force = true + unpublish.execWorkspaces([], [], (err) => { + t.notOk(err) + t.matchSnapshot(result, 'should output all workspaces') + t.end() + }) + }) + + t.test('one workspace --force', (t) => { + npm.localPrefix = testDir + config.force = true + unpublish.execWorkspaces([], ['workspace-a'], (err) => { + t.notOk(err) + t.matchSnapshot(result, 'should output one workspaces') + t.end() + }) + }) + t.end() +}) + +t.test('dryRun with spec', (t) => { + config['dry-run'] = true + const Unpublish = t.mock('../../lib/unpublish.js', { + ...mocks, + libnpmpublish: { unpublish: () => { + throw new Error('should not be called') + } }, + }) + const unpublish = new Unpublish(npm) unpublish.exec(['pkg@1.0.0'], err => { if (err) throw err t.equal( result, - '', - 'should have no output' + '- pkg@1.0.0', + 'should output removed pkg@version on success' + ) + t.end() + }) +}) + +t.test('dryRun with local package', (t) => { + config['dry-run'] = true + config.force = true + const Unpublish = t.mock('../../lib/unpublish.js', { + ...mocks, + libnpmpublish: { unpublish: () => { + throw new Error('should not be called') + } }, + }) + const unpublish = new Unpublish(npm) + unpublish.exec([], err => { + if (err) + throw err + + t.equal( + result, + '- pkg@1.0.0', + 'should output removed pkg@1.0.0 on success' ) t.end() }) @@ -331,7 +401,6 @@ t.test('completion', async t => { } }, }, - 'npm-package-arg': require('npm-package-arg'), 'npm-registry-fetch': { async json () { return { @@ -369,7 +438,6 @@ t.test('completion', async t => { } }, }, - 'npm-package-arg': require('npm-package-arg'), 'npm-registry-fetch': { async json () { return { @@ -403,7 +471,6 @@ t.test('completion', async t => { } }, }, - 'npm-package-arg': require('npm-package-arg'), }) const unpublish = new Unpublish(npm)