fix: npm unstar

lib/star.js

@@ -3,73 +3,75 @@
 const fetch = require('npm-registry-fetch')
 const log = require('npmlog')
 const npa = require('npm-package-arg')
+
 const npm = require('./npm.js')
 const output = require('./utils/output.js')
-const usage = require('./utils/usage.js')
-const getItentity = require('./utils/get-identity')
+const usageUtil = require('./utils/usage.js')
+const getIdentity = require('./utils/get-identity')
+const completion = require('./utils/completion/none.js')
 
-star.usage = usage(
+const usage = usageUtil(
   'star',
   'npm star [<pkg>...]\n' +
   'npm unstar [<pkg>...]'
 )
 
-star.completion = function (opts, cb) {
-  // FIXME: there used to be registry completion here, but it stopped making
-  // sense somewhere around 50,000 packages on the registry
-  cb()
-}
+const cmd = (args, cb) => star(args).then(() => cb()).catch(cb)
+
+const star = async args => {
+  if (!args.length)
+    throw new Error(usage)
+
+  // if we're unstarring, then show an empty star image
+  // otherwise, show the full star image
+  const { unicode } = npm.flatOptions
+  const unstar = npm.config.get('star.unstar')
+  const full = unicode ? '\u2605 ' : '(*)'
+  const empty = unicode ? '\u2606 ' : '( )'
+  const show = unstar ? empty : full
+
+  const pkgs = args.map(npa)
+  for (const pkg of pkgs) {
+    const [username, fullData] = await Promise.all([
+      getIdentity(npm.flatOptions),
+      fetch.json(pkg.escapedName, {
+        ...npm.flatOptions,
+        spec: pkg,
+        query: { write: true },
+        preferOnline: true,
+      }),
+    ])
 
-module.exports = star
-function star (args, cb) {
-  const opts = npm.flatOptions
-  return Promise.resolve().then(() => {
-    if (!args.length)
-      throw new Error(star.usage)
-    // if we're unstarring, then show an empty star image
-    // otherwise, show the full star image
-    const unstar = /^un/.test(npm.command)
-    const full = opts.unicode ? '\u2605 ' : '(*)'
-    const empty = opts.unicode ? '\u2606 ' : '( )'
-    const show = unstar ? empty : full
-    return Promise.all(args.map(npa).map(pkg => {
-      return Promise.all([
-        getItentity(opts),
-        fetch.json(pkg.escapedName, {
-          ...opts,
-          spec: pkg,
-          query: { write: true },
-          preferOnline: true,
-        }),
-      ]).then(([username, fullData]) => {
-        if (!username)
-          throw new Error('You need to be logged in!')
-        const body = {
-          _id: fullData._id,
-          _rev: fullData._rev,
-          users: fullData.users || {},
-        }
+    if (!username)
+      throw new Error('You need to be logged in!')
 
-        if (!unstar) {
-          log.info('star', 'starring', body._id)
-          body.users[username] = true
-          log.verbose('star', 'starring', body)
-        } else {
-          delete body.users[username]
-          log.info('star', 'unstarring', body._id)
-          log.verbose('star', 'unstarring', body)
-        }
-        return fetch.json(pkg.escapedName, {
-          ...opts,
-          spec: pkg,
-          method: 'PUT',
-          body,
-        })
-      }).then(data => {
-        output(show + ' ' + pkg.name)
-        log.verbose('star', data)
-        return data
-      })
-    }))
-  }).then(() => cb(), cb)
+    const body = {
+      _id: fullData._id,
+      _rev: fullData._rev,
+      users: fullData.users || {},
+    }
+
+    if (!unstar) {
+      log.info('star', 'starring', body._id)
+      body.users[username] = true
+      log.verbose('star', 'starring', body)
+    } else {
+      delete body.users[username]
+      log.info('unstar', 'unstarring', body._id)
+      log.verbose('unstar', 'unstarring', body)
+    }
+
+    const data = await fetch.json(pkg.escapedName, {
+      ...npm.flatOptions,
+      spec: pkg,
+      method: 'PUT',
+      body,
+    })
+
+    output(show + ' ' + pkg.name)
+    log.verbose('star', data)
+    return data
+  }
 }
+
+module.exports = Object.assign(cmd, { completion, usage })

lib/unstar.js

@@ -0,0 +1,9 @@
+const { usage, completion } = require('./star.js')
+const npm = require('./npm.js')
+
+const unstar = (args, cb) => {
+  npm.config.set('star.unstar', true)
+  return npm.commands.star(args, cb)
+}
+
+module.exports = Object.assign(unstar, { usage, completion })

lib/utils/cmd-list.js

@@ -12,7 +12,6 @@ const shorthands = {
   c: 'config',
   s: 'search',
   se: 'search',
-  unstar: 'star', // same function
   tst: 'test',
   t: 'test',
   ddp: 'dedupe',
@@ -88,6 +87,7 @@ const cmdList = [
   'publish',
   'star',
   'stars',
+  'unstar',
   'adduser',
   'login', // This is an alias for `adduser` but it can be confusing
   'logout',

tap-snapshots/test-lib-utils-cmd-list.js-TAP.test.js

@@ -94,7 +94,6 @@ Object {
     "udpate": "update",
     "un": "uninstall",
     "unlink": "uninstall",
-    "unstar": "star",
     "up": "update",
     "upgrade": "update",
     "urn": "run-script",
@@ -125,6 +124,7 @@ Object {
     "publish",
     "star",
     "stars",
+    "unstar",
     "adduser",
     "login",
     "logout",
@@ -189,7 +189,6 @@ Object {
     "t": "test",
     "tst": "test",
     "un": "uninstall",
-    "unstar": "star",
     "up": "update",
     "v": "view",
     "why": "explain",

test/lib/star.js

@@ -0,0 +1,144 @@
+const requireInject = require('require-inject')
+const t = require('tap')
+
+let result = ''
+
+const noop = () => null
+const npm = { config: { get () {} }, flatOptions: { unicode: false } }
+const npmFetch = { json: noop }
+const npmlog = { error: noop, info: noop, verbose: noop }
+const mocks = {
+  npmlog,
+  'npm-registry-fetch': npmFetch,
+  '../../lib/npm.js': npm,
+  '../../lib/utils/output.js': (...msg) => {
+    result += msg.join('\n')
+  },
+  '../../lib/utils/get-identity.js': async () => 'foo',
+  '../../lib/utils/usage.js': () => 'usage instructions',
+}
+
+const star = requireInject('../../lib/star.js', mocks)
+
+t.afterEach(cb => {
+  npm.config = { get () {} }
+  npm.flatOptions.unicode = false
+  npmlog.info = noop
+  result = ''
+  cb()
+})
+
+t.test('no args', t => {
+  star([], err => {
+    t.match(
+      err,
+      /usage instructions/,
+      'should throw usage instructions'
+    )
+    t.end()
+  })
+})
+
+t.test('star a package', t => {
+  t.plan(4)
+  const pkgName = '@npmcli/arborist'
+  npmFetch.json = async (uri, opts) => ({
+    _id: pkgName,
+    _rev: 'hash',
+    users: (
+      opts.method === 'PUT'
+        ? { foo: true }
+        : {}
+    ),
+  })
+  npmlog.info = (title, msg, id) => {
+    t.equal(title, 'star', 'should use expected title')
+    t.equal(msg, 'starring', 'should use expected msg')
+    t.equal(id, pkgName, 'should use expected id')
+  }
+  star([pkgName], err => {
+    if (err)
+      throw err
+    t.equal(
+      result,
+      '(*) @npmcli/arborist',
+      'should output starred package msg'
+    )
+  })
+})
+
+t.test('unstar a package', t => {
+  t.plan(4)
+  const pkgName = '@npmcli/arborist'
+  npm.config.get = key => key === 'star.unstar'
+  npmFetch.json = async (uri, opts) => ({
+    _id: pkgName,
+    _rev: 'hash',
+    ...(opts.method === 'PUT'
+      ? {}
+      : { foo: true }
+    ),
+  })
+  npmlog.info = (title, msg, id) => {
+    t.equal(title, 'unstar', 'should use expected title')
+    t.equal(msg, 'unstarring', 'should use expected msg')
+    t.equal(id, pkgName, 'should use expected id')
+  }
+  star([pkgName], err => {
+    if (err)
+      throw err
+    t.equal(
+      result,
+      '( ) @npmcli/arborist',
+      'should output unstarred package msg'
+    )
+  })
+})
+
+t.test('unicode', async t => {
+  t.test('star a package', t => {
+    npm.flatOptions.unicode = true
+    npmFetch.json = async (uri, opts) => ({})
+    star(['pkg'], err => {
+      if (err)
+        throw err
+      t.equal(
+        result,
+        '\u2605  pkg',
+        'should output unicode starred package msg'
+      )
+      t.end()
+    })
+  })
+
+  t.test('unstar a package', t => {
+    npm.flatOptions.unicode = true
+    npm.config.get = key => key === 'star.unstar'
+    npmFetch.json = async (uri, opts) => ({})
+    star(['pkg'], err => {
+      if (err)
+        throw err
+      t.equal(
+        result,
+        '\u2606  pkg',
+        'should output unstarred package msg'
+      )
+      t.end()
+    })
+  })
+})
+
+t.test('logged out user', t => {
+  const star = requireInject('../../lib/star.js', {
+    ...mocks,
+    '../../lib/utils/get-identity.js': async () => undefined,
+  })
+  star(['@npmcli/arborist'], err => {
+    t.match(
+      err,
+      /You need to be logged in/,
+      'should throw login required error'
+    )
+    t.end()
+  })
+})

test/lib/unstar.js

@@ -0,0 +1,28 @@
+const requireInject = require('require-inject')
+const t = require('tap')
+
+t.test('unstar', t => {
+  t.plan(3)
+
+  const unstar = requireInject('../../lib/unstar.js', {
+    '../../lib/npm.js': {
+      config: {
+        set: (key, value) => {
+          t.equal(key, 'star.unstar', 'should set unstar config value')
+          t.equal(value, true, 'should set a truthy value')
+        },
+      },
+      commands: {
+        star: (args, cb) => {
+          t.deepEqual(args, ['pkg'], 'should forward packages')
+          cb()
+        },
+      },
+    },
+  })
+
+  unstar(['pkg'], err => {
+    if (err)
+      throw err
+  })
+})