fix: surface failed package delete during unpublish

test/lib/commands/unpublish.js

@@ -228,6 +228,30 @@ t.test('unpublish <pkg> --force no version set', async t => {
   t.equal(joinedOutput(), '- test-package')
 })
 
+t.test('unpublish <pkg> surfaces a failed whole-package delete', async t => {
+  const { joinedOutput, npm } = await loadMockNpm(t, {
+    config: {
+      force: true,
+      ...auth,
+    },
+  })
+  const registry = new MockRegistry({
+    tap: t,
+    registry: npm.config.get('registry'),
+    authorization: 'test-auth-token',
+  })
+  const manifest = registry.manifest({ name: pkg })
+  await registry.package({ manifest, query: { write: true }, times: 2 })
+  registry.nock.delete(`/${pkg}/-rev/${manifest._rev}`).reply(404)
+
+  await t.rejects(
+    npm.exec('unpublish', ['test-package']),
+    { code: 'E404' },
+    'should reject when the registry delete fails'
+  )
+  t.equal(joinedOutput(), '')
+})
+
 t.test('silent', async t => {
   const { joinedOutput, npm } = await loadMockNpm(t, {
     config: {

workspaces/libnpmpublish/lib/unpublish.js

@@ -31,87 +31,88 @@ const unpublish = async (spec, opts) => {
     spec,
   }
 
+  const pkgUri = spec.escapedName
+  let pkg
   try {
-    const pkgUri = spec.escapedName
-    const pkg = await npmFetch.json(pkgUri, {
+    pkg = await npmFetch.json(pkgUri, {
       ...opts,
       query: { write: true },
     })
+  } catch (err) {
+    if (err.code !== 'E404') {
+      throw err
+    }
 
-    const version = spec.rawSpec
-    const allVersions = pkg.versions || {}
-    const versionData = allVersions[version]
+    return true
+  }
 
-    const rawSpecs = (!spec.rawSpec || spec.rawSpec === '*')
-    const onlyVersion = Object.keys(allVersions).length === 1
-    const noVersions = !Object.keys(allVersions).length
+  const version = spec.rawSpec
+  const allVersions = pkg.versions || {}
+  const versionData = allVersions[version]
 
-    // if missing specific version,
-    // assumed unpublished
-    if (!versionData && !rawSpecs && !noVersions) {
-      return true
-    }
+  const rawSpecs = (!spec.rawSpec || spec.rawSpec === '*')
+  const onlyVersion = Object.keys(allVersions).length === 1
+  const noVersions = !Object.keys(allVersions).length
+
+  // if missing specific version,
+  // assumed unpublished
+  if (!versionData && !rawSpecs && !noVersions) {
+    return true
+  }
+
+  // unpublish all versions of a package:
+  // - no specs supplied "npm unpublish foo"
+  // - all specs ("*") "npm unpublish foo@*"
+  // - there was only one version
+  // - has no versions field on packument
+  if (rawSpecs || onlyVersion || noVersions) {
+    await npmFetch(`${pkgUri}/-rev/${pkg._rev}`, {
+      ...opts,
+      method: 'DELETE',
+      ignoreBody: true,
+    })
+    return true
+  } else {
+    const dist = allVersions[version].dist
+    delete allVersions[version]
+
+    const latestVer = pkg['dist-tags'].latest
 
-    // unpublish all versions of a package:
-    // - no specs supplied "npm unpublish foo"
-    // - all specs ("*") "npm unpublish foo@*"
-    // - there was only one version
-    // - has no versions field on packument
-    if (rawSpecs || onlyVersion || noVersions) {
-      await npmFetch(`${pkgUri}/-rev/${pkg._rev}`, {
-        ...opts,
-        method: 'DELETE',
-        ignoreBody: true,
-      })
-      return true
-    } else {
-      const dist = allVersions[version].dist
-      delete allVersions[version]
-
-      const latestVer = pkg['dist-tags'].latest
-
-      // deleting dist tags associated to version
-      Object.keys(pkg['dist-tags']).forEach(tag => {
-        if (pkg['dist-tags'][tag] === version) {
-          delete pkg['dist-tags'][tag]
-        }
-      })
-
-      if (latestVer === version) {
-        pkg['dist-tags'].latest = Object.keys(
-          allVersions
-        ).sort(semver.compareLoose).pop()
+    // deleting dist tags associated to version
+    Object.keys(pkg['dist-tags']).forEach(tag => {
+      if (pkg['dist-tags'][tag] === version) {
+        delete pkg['dist-tags'][tag]
       }
+    })
 
-      delete pkg._revisions
-      delete pkg._attachments
-
-      // Update packument with removed versions
-      await npmFetch(`${pkgUri}/-rev/${pkg._rev}`, {
-        ...opts,
-        method: 'PUT',
-        body: pkg,
-        ignoreBody: true,
-      })
-
-      // Remove the tarball itself
-      const { _rev } = await npmFetch.json(pkgUri, {
-        ...opts,
-        query: { write: true },
-      })
-      const tarballUrl = getPathname(dist.tarball, opts.registry)
-      await npmFetch(`${tarballUrl}/-rev/${_rev}`, {
-        ...opts,
-        method: 'DELETE',
-        ignoreBody: true,
-      })
-      return true
-    }
-  } catch (err) {
-    if (err.code !== 'E404') {
-      throw err
+    if (latestVer === version) {
+      pkg['dist-tags'].latest = Object.keys(
+        allVersions
+      ).sort(semver.compareLoose).pop()
     }
 
+    delete pkg._revisions
+    delete pkg._attachments
+
+    // Update packument with removed versions
+    await npmFetch(`${pkgUri}/-rev/${pkg._rev}`, {
+      ...opts,
+      method: 'PUT',
+      body: pkg,
+      ignoreBody: true,
+    })
+
+    // Remove the tarball itself
+    const { _rev } = await npmFetch.json(pkgUri, {
+      ...opts,
+      query: { write: true },
+    })
+    const tarballUrl = getPathname(dist.tarball, opts.registry)
+    await npmFetch(`${tarballUrl}/-rev/${_rev}`, {
+      ...opts,
+      method: 'DELETE',
+      ignoreBody: true,
+    })
     return true
   }
 }

workspaces/libnpmpublish/test/unpublish.js

@@ -107,6 +107,22 @@ t.test('404 considered a success', async t => {
   t.ok(ret, '@npmcli/npm-unpublish-test was unpublished')
 })
 
+t.test('404 from whole-package delete is not considered a success', async t => {
+  const registry = new MockRegistry({
+    tap: t,
+    registry: opts.registry,
+  })
+  const manifest = registry.manifest({ name: '@npmcli/npm-unpublish-test' })
+  const spec = npa(manifest.name)
+  registry.package({ manifest, query: { write: true } })
+  registry.nock.delete(`/${spec.escapedName}/-rev/${manifest._rev}`).reply(404)
+  await t.rejects(
+    unpublish('@npmcli/npm-unpublish-test', opts),
+    { code: 'E404' },
+    'surfaces a failed full-package delete'
+  )
+})
+
 t.test('non-404 errors', async t => {
   const registry = new MockRegistry({
     tap: t,