fix: minor tweaks to lib/unpublish.js

- Fix handling missing files error on reading package.json
- Fixes autocompletion
- Fixes printing name and version when using no args
- Adds `test/lib/unpublish.js` tests

Fixes: https://github.com/npm/statusboard/issues/180

PR-URL: https://github.com/npm/cli/pull/2304
Credit: @ruyadorno
Close: #2304
Reviewed-by: @isaacs

2 files changed, 537 insertions, 24 deletions

diff --git a/lib/unpublish.js b/lib/unpublish.js
index 4d05627d2..d6dbc8d6e 100644
--- a/lib/unpublish.js
+++ b/lib/unpublish.js
@@ -1,3 +1,5 @@
+'use strict'
+
 const path = require('path')
 const util = require('util')
 const log = require('npmlog')
@@ -11,7 +13,7 @@ const npm = require('./npm.js')
 const usageUtil = require('./utils/usage.js')
 const output = require('./utils/output.js')
 const otplease = require('./utils/otplease.js')
-const whoami = util.promisify(require('./whoami.js'))
+const getIdentity = require('./utils/get-identity.js')
 
 const usage = usageUtil('unpublish', 'npm unpublish [<@scope>/]<pkg>[@<version>]')
 
@@ -25,18 +27,18 @@ const completionFn = async (args) => {
   const { partialWord, conf } = args
 
   if (conf.argv.remain.length >= 3)
-    return
+    return []
 
-  const username = await whoami([], true)
+  const opts = npm.flatOptions
+  const username = await getIdentity({ ...opts }).catch(() => null)
   if (!username)
     return []
-  const opts = npm.flatOptions
 
   const access = await libaccess.lsPackages(username, opts)
   // do a bit of filtering at this point, so that we don't need
   // to fetch versions for more than one thing, but also don't
   // accidentally a whole project
-  let pkgs = Object.keys(access)
+  let pkgs = Object.keys(access || {})
   if (!partialWord || !pkgs.length)
     return pkgs
 
@@ -55,18 +57,20 @@ const completionFn = async (args) => {
 
 async function unpublish (args) {
   if (args.length > 1)
-    throw usage
+    throw new Error(usage)
 
   const spec = args.length && npa(args[0])
   const opts = npm.flatOptions
   const { force, silent, loglevel } = opts
-  let ret
+  let res
+  let pkgName
+  let pkgVersion
 
   log.silly('unpublish', 'args[0]', args[0])
   log.silly('unpublish', 'spec', spec)
 
   if (!spec.rawSpec && !force) {
-    throw (
+    throw new Error(
       'Refusing to delete entire project.\n' +
       'Run with --force to do this.\n' +
       usage
@@ -77,31 +81,34 @@ async function unpublish (args) {
     // if there's a package.json in the current folder, then
     // read the package name and version out of that.
     const pkgJson = path.join(npm.localPrefix, 'package.json')
-    const manifest = await readJson(pkgJson)
-
-    log.verbose('unpublish', manifest)
-
-    const { name, version, publishConfig } = manifest
-    const pkgJsonSpec = npa.resolve(name, version)
-
+    let manifest
     try {
-      ret = await otplease(opts, opts => libunpub(pkgJsonSpec, { ...opts, publishConfig }))
+      manifest = await readJson(pkgJson)
     } catch (err) {
       if (err && err.code !== 'ENOENT' && err.code !== 'ENOTDIR')
         throw err
       else
-        throw `Usage: ${usage}`
+        throw new Error(`Usage: ${usage}`)
     }
-  } else
-    ret = await otplease(opts, opts => libunpub(spec, opts))
 
-  if (!silent && loglevel !== 'silent') {
-    output(`- ${spec.name}${
-      spec.type === 'version' ? `@${spec.rawSpec}` : ''
-    }`)
+    log.verbose('unpublish', manifest)
+
+    const { name, version, publishConfig } = manifest
+    const pkgJsonSpec = npa.resolve(name, version)
+
+    res = await otplease(opts, opts => libunpub(pkgJsonSpec, { ...opts, publishConfig }))
+    pkgName = name
+    pkgVersion = version ? `@${version}` : ''
+  } else {
+    res = await otplease(opts, opts => libunpub(spec, opts))
+    pkgName = spec.name
+    pkgVersion = spec.type === 'version' ? `@${spec.rawSpec}` : ''
   }
 
-  return ret
+  if (!silent && loglevel !== 'silent')
+    output(`- ${pkgName}${pkgVersion}`)
+
+  return res
 }
 
 module.exports = Object.assign(cmd, { completion, usage })
diff --git a/test/lib/unpublish.js b/test/lib/unpublish.js
new file mode 100644
index 000000000..11e24714d
--- /dev/null
+++ b/test/lib/unpublish.js
@@ -0,0 +1,506 @@
+const t = require('tap')
+const requireInject = require('require-inject')
+
+let result = ''
+const noop = () => null
+const npm = {
+  localPrefix: '',
+  flatOptions: {
+    force: false,
+    silent: false,
+    loglevel: 'silly',
+  },
+}
+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/npm.js': npm,
+  '../../lib/utils/output.js': (...msg) => {
+    result += msg.join('\n')
+  },
+  '../../lib/utils/otplease.js': async (opts, fn) => fn(opts),
+  '../../lib/utils/usage.js': () => 'usage instructions',
+  '../../lib/utils/get-identity.js': async () => 'foo',
+}
+
+t.afterEach(cb => {
+  result = ''
+  npm.flatOptions.force = false
+  npm.flatOptions.loglevel = 'silly'
+  npm.flatOptions.silent = false
+  cb()
+})
+
+t.test('no args --force', t => {
+  t.plan(9)
+
+  npm.flatOptions.force = true
+
+  const npmlog = {
+    silly (title) {
+      t.equal(title, 'unpublish', 'should silly log args')
+    },
+    verbose (title, msg) {
+      t.equal(title, 'unpublish', 'should have expected title')
+      t.match(
+        msg,
+        { name: 'pkg', version: '1.0.0' },
+        'should have msg printing package.json contents'
+      )
+    },
+  }
+
+  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.deepEqual(
+        opts,
+        {
+          force: true,
+          silent: false,
+          loglevel: 'silly',
+          publishConfig: undefined,
+        },
+        'should unpublish with expected opts'
+      )
+    },
+  }
+
+  const unpublish = requireInject('../../lib/unpublish.js', {
+    ...mocks,
+    npmlog,
+    libnpmpublish,
+    'npm-package-arg': npa,
+    'read-package-json': (path, cb) => cb(null, {
+      name: 'pkg',
+      version: '1.0.0',
+    }),
+  })
+
+  unpublish([], err => {
+    if (err)
+      throw err
+
+    t.equal(
+      result,
+      '- pkg@1.0.0',
+      'should output removed pkg@version on success'
+    )
+  })
+})
+
+t.test('no args --force missing package.json', t => {
+  npm.flatOptions.force = true
+
+  const unpublish = requireInject('../../lib/unpublish.js', {
+    ...mocks,
+    'read-package-json': (path, cb) => cb(Object.assign(
+      new Error('ENOENT'),
+      { code: 'ENOENT' }
+    )),
+  })
+
+  unpublish([], err => {
+    t.match(
+      err,
+      /usage instructions/,
+      'should throw usage instructions on missing package.json'
+    )
+    t.end()
+  })
+})
+
+t.test('no args --force unknown error reading package.json', t => {
+  npm.flatOptions.force = true
+
+  const unpublish = requireInject('../../lib/unpublish.js', {
+    ...mocks,
+    'read-package-json': (path, cb) => cb(new Error('ERR')),
+  })
+
+  unpublish([], err => {
+    t.match(
+      err,
+      /ERR/,
+      'should throw unknown error from reading package.json'
+    )
+    t.end()
+  })
+})
+
+t.test('no args', t => {
+  const unpublish = requireInject('../../lib/unpublish.js', {
+    ...mocks,
+  })
+
+  unpublish([], err => {
+    t.match(
+      err,
+      /Refusing to delete entire project/,
+      'should throw --force required error on no args'
+    )
+    t.end()
+  })
+})
+
+t.test('too many args', t => {
+  const unpublish = requireInject('../../lib/unpublish.js', {
+    ...mocks,
+  })
+
+  unpublish(['a', 'b'], err => {
+    t.match(
+      err,
+      /usage instructions/,
+      'should throw usage instructions if too many args'
+    )
+    t.end()
+  })
+})
+
+t.test('unpublish <pkg>@version', t => {
+  t.plan(7)
+
+  const pa = {
+    name: 'pkg',
+    rawSpec: '1.0.0',
+    type: 'version',
+  }
+
+  const npmlog = {
+    silly (title, key, value) {
+      t.equal(title, 'unpublish', 'should silly log args')
+      if (key === 'spec')
+        t.equal(value, pa, 'should log parsed npa object')
+      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.deepEqual(
+        opts,
+        {
+          force: false,
+          silent: false,
+          loglevel: 'silly',
+        },
+        'should unpublish with expected opts'
+      )
+    },
+  }
+
+  const unpublish = requireInject('../../lib/unpublish.js', {
+    ...mocks,
+    npmlog,
+    libnpmpublish,
+    'npm-package-arg': npa,
+  })
+
+  unpublish(['pkg@1.0.0'], err => {
+    if (err)
+      throw err
+
+    t.equal(
+      result,
+      '- pkg@1.0.0',
+      'should output removed pkg@version on success'
+    )
+  })
+})
+
+t.test('no version found in package.json', t => {
+  npm.flatOptions.force = true
+
+  const npa = () => ({
+    name: 'pkg',
+    type: 'version',
+  })
+
+  npa.resolve = () => ''
+
+  const unpublish = requireInject('../../lib/unpublish.js', {
+    ...mocks,
+    'npm-package-arg': npa,
+    'read-package-json': (path, cb) => cb(null, {
+      name: 'pkg',
+    }),
+  })
+
+  unpublish([], err => {
+    if (err)
+      throw err
+
+    t.equal(
+      result,
+      '- pkg',
+      'should output removed pkg on success'
+    )
+    t.end()
+  })
+})
+
+t.test('unpublish <pkg> --force no version set', t => {
+  npm.flatOptions.force = true
+
+  const unpublish = requireInject('../../lib/unpublish.js', {
+    ...mocks,
+    'npm-package-arg': () => ({
+      name: 'pkg',
+      rawSpec: '',
+      type: 'tag',
+    }),
+  })
+
+  unpublish(['pkg'], err => {
+    if (err)
+      throw err
+
+    t.equal(
+      result,
+      '- pkg',
+      'should output pkg removed'
+    )
+    t.end()
+  })
+})
+
+t.test('silent', t => {
+  npm.flatOptions.loglevel = 'silent'
+
+  const npa = () => ({
+    name: 'pkg',
+    rawSpec: '1.0.0',
+    type: 'version',
+  })
+
+  npa.resolve = () => ''
+
+  const unpublish = requireInject('../../lib/unpublish.js', {
+    ...mocks,
+    'npm-package-arg': npa,
+  })
+
+  unpublish(['pkg@1.0.0'], err => {
+    if (err)
+      throw err
+
+    t.equal(
+      result,
+      '',
+      'should have no output'
+    )
+    t.end()
+  })
+})
+
+t.test('completion', t => {
+  const testComp = (t, { completion, argv, partialWord, expect, title }) =>
+    new Promise((resolve, rej) => {
+      completion({conf: {argv: {remain: argv}}, partialWord}, (er, res) => {
+        if (er)
+          rej(er)
+        t.strictSame(res, expect, title || argv.join(' '))
+        resolve()
+      })
+    })
+
+  t.test('completing with multiple versions from the registry', async t => {
+    const { completion } = requireInject('../../lib/unpublish.js', {
+      ...mocks,
+      libnpmaccess: {
+        async lsPackages () {
+          return {
+            pkg: 'write',
+            bar: 'write',
+          }
+        },
+      },
+      'npm-package-arg': require('npm-package-arg'),
+      'npm-registry-fetch': {
+        async json () {
+          return {
+            versions: {
+              '1.0.0': {},
+              '1.0.1': {},
+              '2.0.0': {},
+            },
+          }
+        },
+      },
+    })
+
+    await testComp(t, {
+      completion,
+      argv: ['npm', 'unpublish'],
+      partialWord: 'pkg',
+      expect: [
+        'pkg@1.0.0',
+        'pkg@1.0.1',
+        'pkg@2.0.0',
+      ],
+    })
+  })
+
+  t.test('no versions retrieved', async t => {
+    const { completion } = requireInject('../../lib/unpublish.js', {
+      ...mocks,
+      libnpmaccess: {
+        async lsPackages () {
+          return {
+            pkg: 'write',
+            bar: 'write',
+          }
+        },
+      },
+      'npm-package-arg': require('npm-package-arg'),
+      'npm-registry-fetch': {
+        async json () {
+          return {
+            versions: {},
+          }
+        },
+      },
+    })
+
+    await testComp(t, {
+      completion,
+      argv: ['npm', 'unpublish'],
+      partialWord: 'pkg',
+      expect: [
+        'pkg',
+      ],
+      title: 'should autocomplete package name only',
+    })
+  })
+
+  t.test('packages starting with same letters', async t => {
+    const { completion } = requireInject('../../lib/unpublish.js', {
+      ...mocks,
+      libnpmaccess: {
+        async lsPackages () {
+          return {
+            pkg: 'write',
+            pkga: 'write',
+            pkgb: 'write',
+          }
+        },
+      },
+      'npm-package-arg': require('npm-package-arg'),
+    })
+
+    await testComp(t, {
+      completion,
+      argv: ['npm', 'unpublish'],
+      partialWord: 'pkg',
+      expect: [
+        'pkg',
+        'pkga',
+        'pkgb',
+      ],
+    })
+  })
+
+  t.test('no packages retrieved', async t => {
+    const { completion } = requireInject('../../lib/unpublish.js', {
+      ...mocks,
+      libnpmaccess: {
+        async lsPackages () {
+          return {}
+        },
+      },
+    })
+
+    await testComp(t, {
+      completion,
+      argv: ['npm', 'unpublish'],
+      partialWord: 'pkg',
+      expect: [],
+      title: 'should have no autocompletion',
+    })
+  })
+
+  t.test('no pkg name to complete', async t => {
+    const { completion } = requireInject('../../lib/unpublish.js', {
+      ...mocks,
+      libnpmaccess: {
+        async lsPackages () {
+          return {
+            pkg: {},
+            bar: {},
+          }
+        },
+      },
+    })
+
+    await testComp(t, {
+      completion,
+      argv: ['npm', 'unpublish'],
+      partialWord: undefined,
+      expect: ['pkg', 'bar'],
+      title: 'should autocomplete with available package names from user',
+    })
+  })
+
+  t.test('no pkg names retrieved from user account', async t => {
+    const { completion } = requireInject('../../lib/unpublish.js', {
+      ...mocks,
+      libnpmaccess: {
+        async lsPackages () {
+          return null
+        },
+      },
+    })
+
+    await testComp(t, {
+      completion,
+      argv: ['npm', 'unpublish'],
+      partialWord: 'pkg',
+      expect: [],
+      title: 'should have no autocomplete',
+    })
+  })
+
+  t.test('logged out user', async t => {
+    const { completion } = requireInject('../../lib/unpublish.js', {
+      ...mocks,
+      '../../lib/utils/get-identity.js': () => Promise.reject(new Error('ERR')),
+    })
+
+    await testComp(t, {
+      completion,
+      argv: ['npm', 'unpublish'],
+      partialWord: 'pkg',
+      expect: [],
+    })
+  })
+
+  t.test('too many args', async t => {
+    const { completion } = requireInject('../../lib/unpublish.js', mocks)
+
+    await testComp(t, {
+      completion,
+      argv: ['npm', 'unpublish', 'foo'],
+      partialWord: undefined,
+      expect: [],
+    })
+  })
+
+  t.end()
+})