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

Author: wraithgar

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,

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,28 +59,43 @@ 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 {
       return versions.map(v => `${pkgs[0]}@${v}`)
     }
   }
 
-  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 })
     }
   }
 }

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 <pkg>@version not the last version', async t => {
   const { joinedOutput, npm } = await loadMockNpm(t, {
     config: {
@@ -129,7 +149,24 @@ t.test('unpublish <pkg>@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(), '- [email protected]')
@@ -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])
 })