From be4741f5bc20239f11842f780047d91fda23935d Mon Sep 17 00:00:00 2001 From: Gar Date: Fri, 1 Dec 2023 09:30:26 -0800 Subject: [PATCH] fix: unpublish bugfixes (#7039) - Properly work in workspace mode. Previously setting the workspace meant the entire package was unpublished. Now it follows the convention of acting as if you were running from the workspace directory. - Better checking of when you are unpublishing the last version of a package. npm checks the actual manifest and compares it to the version you are asking to unpublish. - Error on ranges and tags. npm doesn't unpublish ranges or tags, and giving those as inputs would give unexepected results. - Proper output of what was unpublished. Previously the package (and sometimes version) displayed would not match what was actually unpublished. - Updated docs specifying that unpublishing with no parameters will only unpublish the version represented by the local package.json --- docs/lib/content/commands/npm-unpublish.md | 8 +- lib/commands/unpublish.js | 103 +++++++++++---------- test/lib/commands/unpublish.js | 103 +++++++++++++-------- 3 files changed, 126 insertions(+), 88 deletions(-) diff --git a/docs/lib/content/commands/npm-unpublish.md b/docs/lib/content/commands/npm-unpublish.md index 82c4424c90d38..741fc83cee9aa 100644 --- a/docs/lib/content/commands/npm-unpublish.md +++ b/docs/lib/content/commands/npm-unpublish.md @@ -25,8 +25,12 @@ removing the tarball. The npm registry will return an error if you are not [logged in](/commands/npm-adduser). -If you do not specify a version or if you remove all of a package's -versions then the registry will remove the root package entry entirely. +If you do not specify a package name at all, the name and version to be +unpublished will be pulled from the project in the current directory. + +If you specify a package name but do not specify a version or if you +remove all of a package's versions then the registry will remove the +root package entry entirely. Even if you unpublish a package version, that specific name and version combination can never be reused. In order to publish the package again, diff --git a/lib/commands/unpublish.js b/lib/commands/unpublish.js index 402f8f30efff8..f5568a3540e12 100644 --- a/lib/commands/unpublish.js +++ b/lib/commands/unpublish.js @@ -1,7 +1,7 @@ const libaccess = require('libnpmaccess') const libunpub = require('libnpmpublish').unpublish const npa = require('npm-package-arg') -const npmFetch = require('npm-registry-fetch') +const pacote = require('pacote') const pkgJson = require('@npmcli/package-json') const { flatten } = require('@npmcli/config/lib/definitions') @@ -23,12 +23,12 @@ class Unpublish extends BaseCommand { static ignoreImplicitWorkspace = false static async getKeysOfVersions (name, opts) { - const pkgUri = npa(name).escapedName - const json = await npmFetch.json(`${pkgUri}?write=true`, { + const packument = await pacote.packument(name, { ...opts, spec: name, + query: { write: true }, }) - return Object.keys(json.versions) + return Object.keys(packument.versions) } static async completion (args, npm) { @@ -59,7 +59,7 @@ class Unpublish extends BaseCommand { return pkgs } - const versions = await this.getKeysOfVersions(pkgs[0], opts) + const versions = await Unpublish.getKeysOfVersions(pkgs[0], opts) if (!versions.length) { return pkgs } else { @@ -67,20 +67,35 @@ class Unpublish extends BaseCommand { } } - async exec (args) { + async exec (args, { localPrefix } = {}) { if (args.length > 1) { throw this.usageError() } - let spec = args.length && npa(args[0]) + // workspace mode + if (!localPrefix) { + localPrefix = this.npm.localPrefix + } + const force = this.npm.config.get('force') const { silent } = this.npm const dryRun = this.npm.config.get('dry-run') + let spec + if (args.length) { + spec = npa(args[0]) + if (spec.type !== 'version' && spec.rawSpec !== '*') { + throw this.usageError( + 'Can only unpublish a single version, or the entire project.\n' + + 'Tags and ranges are not supported.' + ) + } + } + log.silly('unpublish', 'args[0]', args[0]) log.silly('unpublish', 'spec', spec) - if ((!spec || !spec.rawSpec) && !force) { + if (spec?.rawSpec === '*' && !force) { throw this.usageError( 'Refusing to delete entire project.\n' + 'Run with --force to do this.' @@ -89,69 +104,63 @@ class Unpublish extends BaseCommand { const opts = { ...this.npm.flatOptions } - let pkgName - let pkgVersion let manifest - let manifestErr try { - const { content } = await pkgJson.prepare(this.npm.localPrefix) + const { content } = await pkgJson.prepare(localPrefix) manifest = content } catch (err) { - manifestErr = err - } - if (spec) { - // If cwd has a package.json with a name that matches the package being - // unpublished, load up the publishConfig - if (manifest && manifest.name === spec.name && manifest.publishConfig) { - flatten(manifest.publishConfig, opts) - } - const versions = await Unpublish.getKeysOfVersions(spec.name, opts) - if (versions.length === 1 && !force) { - throw this.usageError(LAST_REMAINING_VERSION_ERROR) - } - pkgName = spec.name - pkgVersion = spec.type === 'version' ? `@${spec.rawSpec}` : '' - } else { - if (manifestErr) { - if (manifestErr.code === 'ENOENT' || manifestErr.code === 'ENOTDIR') { + // we needed the manifest to figure out the package to unpublish + if (!spec) { + if (err.code === 'ENOENT' || err.code === 'ENOTDIR') { throw this.usageError() } else { - throw manifestErr + throw err } } + } - log.verbose('unpublish', manifest) - + let pkgVersion // for cli output + if (spec) { + pkgVersion = spec.type === 'version' ? `@${spec.rawSpec}` : '' + } else { spec = npa.resolve(manifest.name, manifest.version) - if (manifest.publishConfig) { - flatten(manifest.publishConfig, opts) + log.verbose('unpublish', manifest) + pkgVersion = manifest.version ? `@${manifest.version}` : '' + if (!manifest.version && !force) { + throw this.usageError( + 'Refusing to delete entire project.\n' + + 'Run with --force to do this.' + ) } + } - pkgName = manifest.name - pkgVersion = manifest.version ? `@${manifest.version}` : '' + // If localPrefix has a package.json with a name that matches the package + // being unpublished, load up the publishConfig + if (manifest?.name === spec.name && manifest.publishConfig) { + flatten(manifest.publishConfig, opts) + } + + const versions = await Unpublish.getKeysOfVersions(spec.name, opts) + if (versions.length === 1 && spec.rawSpec === versions[0] && !force) { + throw this.usageError(LAST_REMAINING_VERSION_ERROR) + } + if (versions.length === 1) { + pkgVersion = '' } if (!dryRun) { await otplease(this.npm, opts, o => libunpub(spec, o)) } if (!silent) { - this.npm.output(`- ${pkgName}${pkgVersion}`) + this.npm.output(`- ${spec.name}${pkgVersion}`) } } async execWorkspaces (args) { await this.setWorkspaces() - 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 this.workspaceNames) { - await this.exec([name]) + for (const path of this.workspacePaths) { + await this.exec(args, { localPrefix: path }) } } } diff --git a/test/lib/commands/unpublish.js b/test/lib/commands/unpublish.js index 6e898bd3d07e4..044b2cf8c546a 100644 --- a/test/lib/commands/unpublish.js +++ b/test/lib/commands/unpublish.js @@ -26,10 +26,10 @@ t.test('no args --force success', async t => { authorization: 'test-auth-token', }) const manifest = registry.manifest({ name: pkg }) - await registry.package({ manifest, query: { write: true } }) + await registry.package({ manifest, query: { write: true }, times: 2 }) registry.unpublish({ manifest }) await npm.exec('unpublish', []) - t.equal(joinedOutput(), '- test-package@1.0.0') + t.equal(joinedOutput(), '- test-package') }) t.test('no args --force missing package.json', async t => { @@ -63,11 +63,11 @@ t.test('no args --force error reading package.json', async t => { ) }) -t.test('no args entire project', async t => { +t.test('no force entire project', async t => { const { npm } = await loadMockNpm(t) await t.rejects( - npm.exec('unpublish', []), + npm.exec('unpublish', ['@npmcli/unpublish-test']), /Refusing to delete entire project/ ) }) @@ -82,6 +82,26 @@ t.test('too many args', async t => { ) }) +t.test('range', async t => { + const { npm } = await loadMockNpm(t) + + await t.rejects( + npm.exec('unpublish', ['a@>1.0.0']), + { code: 'EUSAGE' }, + /single version/ + ) +}) + +t.test('tag', async t => { + const { npm } = await loadMockNpm(t) + + await t.rejects( + npm.exec('unpublish', ['a@>1.0.0']), + { code: 'EUSAGE' }, + /single version/ + ) +}) + t.test('unpublish @version not the last version', async t => { const { joinedOutput, npm } = await loadMockNpm(t, { config: { @@ -129,7 +149,24 @@ t.test('unpublish @version last version', async t => { ) }) -t.test('no version found in package.json', async t => { +t.test('no version found in package.json no force', async t => { + const { npm } = await loadMockNpm(t, { + config: { + ...auth, + }, + prefixDir: { + 'package.json': JSON.stringify({ + name: pkg, + }, null, 2), + }, + }) + await t.rejects( + npm.exec('unpublish', []), + /Refusing to delete entire project/ + ) +}) + +t.test('no version found in package.json with force', async t => { const { joinedOutput, npm } = await loadMockNpm(t, { config: { force: true, @@ -147,7 +184,7 @@ t.test('no version found in package.json', async t => { authorization: 'test-auth-token', }) const manifest = registry.manifest({ name: pkg }) - await registry.package({ manifest, query: { write: true } }) + await registry.package({ manifest, query: { write: true }, times: 2 }) registry.unpublish({ manifest }) await npm.exec('unpublish', []) @@ -219,7 +256,7 @@ t.test('workspaces', async t => { 'workspace-b': { 'package.json': JSON.stringify({ name: 'workspace-b', - version: '1.2.3-n', + version: '1.2.3-b', repository: 'https://github.com/npm/workspace-b', }), }, @@ -231,20 +268,20 @@ t.test('workspaces', async t => { }, } - t.test('no force', async t => { + t.test('with package name no force', async t => { const { npm } = await loadMockNpm(t, { config: { - workspaces: true, + workspace: ['workspace-a'], }, prefixDir, }) await t.rejects( - npm.exec('unpublish', []), + npm.exec('unpublish', ['workspace-a']), /Refusing to delete entire project/ ) }) - t.test('all workspaces --force', async t => { + t.test('all workspaces last version --force', async t => { const { joinedOutput, npm } = await loadMockNpm(t, { config: { workspaces: true, @@ -258,9 +295,9 @@ t.test('workspaces', async t => { registry: npm.config.get('registry'), authorization: 'test-auth-token', }) - const manifestA = registry.manifest({ name: 'workspace-a' }) - const manifestB = registry.manifest({ name: 'workspace-b' }) - const manifestN = registry.manifest({ name: 'workspace-n' }) + const manifestA = registry.manifest({ name: 'workspace-a', versions: ['1.2.3-a'] }) + const manifestB = registry.manifest({ name: 'workspace-b', versions: ['1.2.3-b'] }) + const manifestN = registry.manifest({ name: 'workspace-n', versions: ['1.2.3-n'] }) await registry.package({ manifest: manifestA, query: { write: true }, times: 2 }) await registry.package({ manifest: manifestB, query: { write: true }, times: 2 }) await registry.package({ manifest: manifestN, query: { write: true }, times: 2 }) @@ -271,28 +308,6 @@ t.test('workspaces', async t => { await npm.exec('unpublish', []) t.equal(joinedOutput(), '- workspace-a\n- workspace-b\n- workspace-n') }) - - t.test('one workspace --force', async t => { - const { joinedOutput, npm } = await loadMockNpm(t, { - config: { - workspace: ['workspace-a'], - force: true, - ...auth, - }, - prefixDir, - }) - const registry = new MockRegistry({ - tap: t, - registry: npm.config.get('registry'), - authorization: 'test-auth-token', - }) - const manifestA = registry.manifest({ name: 'workspace-a' }) - await registry.package({ manifest: manifestA, query: { write: true }, times: 2 }) - registry.nock.delete(`/workspace-a/-rev/${manifestA._rev}`).reply(201) - - await npm.exec('unpublish', []) - t.equal(joinedOutput(), '- workspace-a') - }) }) t.test('dryRun with spec', async t => { @@ -331,6 +346,16 @@ t.test('dryRun with no args', async t => { }, null, 2), }, }) + const registry = new MockRegistry({ + tap: t, + registry: npm.config.get('registry'), + authorization: 'test-auth-token', + }) + const manifest = registry.manifest({ + name: pkg, + packuments: ['1.0.0', '1.0.1'], + }) + await registry.package({ manifest, query: { write: true } }) await npm.exec('unpublish', []) t.equal(joinedOutput(), '- test-package@1.0.0') @@ -360,10 +385,10 @@ t.test('publishConfig no spec', async t => { authorization: 'test-other-token', }) const manifest = registry.manifest({ name: pkg }) - await registry.package({ manifest, query: { write: true } }) + await registry.package({ manifest, query: { write: true }, times: 2 }) registry.unpublish({ manifest }) await npm.exec('unpublish', []) - t.equal(joinedOutput(), '- test-package@1.0.0') + t.equal(joinedOutput(), '- test-package') }) t.test('publishConfig with spec', async t => { @@ -421,7 +446,7 @@ t.test('scoped registry config', async t => { authorization: 'test-other-token', }) const manifest = registry.manifest({ name: scopedPkg }) - await registry.package({ manifest, query: { write: true } }) + await registry.package({ manifest, query: { write: true }, times: 2 }) registry.unpublish({ manifest }) await npm.exec('unpublish', [scopedPkg]) })