fix: do less work looking up commands (#6283)

Author: wraithgar

lib/commands/completion.js

@@ -34,9 +34,7 @@ const nopt = require('nopt')
 const { resolve } = require('path')
 
 const { definitions, shorthands } = require('../utils/config/index.js')
-const { aliases, commands, plumbing } = require('../utils/cmd-list.js')
-const aliasNames = Object.keys(aliases)
-const fullList = commands.concat(aliasNames).filter(c => !plumbing.includes(c))
+const { commands, aliases } = require('../utils/cmd-list.js')
 const configNames = Object.keys(definitions)
 const shorthandNames = Object.keys(shorthands)
 const allConfs = configNames.concat(shorthandNames)
@@ -263,7 +261,8 @@ const isFlag = word => {
 // complete against the npm commands
 // if they all resolve to the same thing, just return the thing it already is
 const cmdCompl = (opts, npm) => {
-  const matches = fullList.filter(c => c.startsWith(opts.partialWord))
+  const allCommands = commands.concat(Object.keys(aliases))
+  const matches = allCommands.filter(c => c.startsWith(opts.partialWord))
   if (!matches.length) {
     return matches
   }
@@ -273,7 +272,7 @@ const cmdCompl = (opts, npm) => {
     return [...derefs]
   }
 
-  return fullList
+  return allCommands
 }
 
 module.exports = Completion

lib/npm.js

@@ -5,6 +5,7 @@ const Config = require('@npmcli/config')
 const chalk = require('chalk')
 const which = require('which')
 const fs = require('fs/promises')
+const abbrev = require('abbrev')
 
 // Patch the global fs module here at the app level
 require('graceful-fs').gracefulify(require('fs'))
@@ -18,7 +19,7 @@ const log = require('./utils/log-shim')
 const replaceInfo = require('./utils/replace-info.js')
 const updateNotifier = require('./utils/update-notifier.js')
 const pkg = require('../package.json')
-const cmdList = require('./utils/cmd-list.js')
+const { commands, aliases } = require('./utils/cmd-list.js')
 
 class Npm extends EventEmitter {
   static get version () {
@@ -84,18 +85,30 @@ class Npm extends EventEmitter {
     if (!c) {
       return
     }
+
+    // Translate camelCase to snake-case (i.e. installTest to install-test)
     if (c.match(/[A-Z]/)) {
       c = c.replace(/([A-Z])/g, m => '-' + m.toLowerCase())
     }
-    if (cmdList.plumbing.indexOf(c) !== -1) {
+
+    // if they asked for something exactly we are done
+    if (commands.includes(c)) {
       return c
     }
+
+    // if they asked for a direct alias
+    if (aliases[c]) {
+      return aliases[c]
+    }
+
+    const abbrevs = abbrev(commands.concat(Object.keys(aliases)))
+
     // first deref the abbrev, if there is one
     // then resolve any aliases
     // so `npm install-cl` will resolve to `install-clean` then to `ci`
-    let a = cmdList.abbrevs[c]
-    while (cmdList.aliases[a]) {
-      a = cmdList.aliases[a]
+    let a = abbrevs[c]
+    while (aliases[a]) {
+      a = aliases[a]
     }
     return a
   }

lib/utils/cmd-list.js

@@ -1,74 +1,5 @@
-const abbrev = require('abbrev')
-const localeCompare = require('@isaacs/string-locale-compare')('en')
-
-// plumbing should not have any aliases
-const aliases = {
-
-  // aliases
-  author: 'owner',
-  home: 'docs',
-  issues: 'bugs',
-  info: 'view',
-  show: 'view',
-  find: 'search',
-  add: 'install',
-  unlink: 'uninstall',
-  remove: 'uninstall',
-  rm: 'uninstall',
-  r: 'uninstall',
-
-  // short names for common things
-  un: 'uninstall',
-  rb: 'rebuild',
-  list: 'ls',
-  ln: 'link',
-  create: 'init',
-  i: 'install',
-  it: 'install-test',
-  cit: 'install-ci-test',
-  up: 'update',
-  c: 'config',
-  s: 'search',
-  se: 'search',
-  tst: 'test',
-  t: 'test',
-  ddp: 'dedupe',
-  v: 'view',
-  run: 'run-script',
-  'clean-install': 'ci',
-  'clean-install-test': 'install-ci-test',
-  x: 'exec',
-  why: 'explain',
-  la: 'll',
-  verison: 'version',
-  ic: 'ci',
-
-  // typos
-  innit: 'init',
-  // manually abbrev so that install-test doesn't make insta stop working
-  in: 'install',
-  ins: 'install',
-  inst: 'install',
-  insta: 'install',
-  instal: 'install',
-  isnt: 'install',
-  isnta: 'install',
-  isntal: 'install',
-  isntall: 'install',
-  'install-clean': 'ci',
-  'isntall-clean': 'ci',
-  hlep: 'help',
-  'dist-tags': 'dist-tag',
-  upgrade: 'update',
-  udpate: 'update',
-  rum: 'run-script',
-  sit: 'install-ci-test',
-  urn: 'run-script',
-  ogr: 'org',
-  'add-user': 'adduser',
-}
-
-// these are filenames in .
+// These correspond to filenames in lib/commands
+// Please keep this list sorted alphabetically
 const commands = [
   'access',
   'adduser',
@@ -92,14 +23,15 @@ const commands = [
   'fund',
   'get',
   'help',
+  'help-search',
   'hook',
   'init',
   'install',
   'install-ci-test',
   'install-test',
   'link',
   'll',
-  'login', // This is an alias for `adduser` but it can be confusing
+  'login',
   'logout',
   'ls',
   'org',
@@ -135,16 +67,76 @@ const commands = [
   'version',
   'view',
   'whoami',
-].sort(localeCompare)
+]
+
+// These must resolve to an entry in commands
+const aliases = {
+
+  // aliases
+  author: 'owner',
+  home: 'docs',
+  issues: 'bugs',
+  info: 'view',
+  show: 'view',
+  find: 'search',
+  add: 'install',
+  unlink: 'uninstall',
+  remove: 'uninstall',
+  rm: 'uninstall',
+  r: 'uninstall',
+
+  // short names for common things
+  un: 'uninstall',
+  rb: 'rebuild',
+  list: 'ls',
+  ln: 'link',
+  create: 'init',
+  i: 'install',
+  it: 'install-test',
+  cit: 'install-ci-test',
+  up: 'update',
+  c: 'config',
+  s: 'search',
+  se: 'search',
+  tst: 'test',
+  t: 'test',
+  ddp: 'dedupe',
+  v: 'view',
+  run: 'run-script',
+  'clean-install': 'ci',
+  'clean-install-test': 'install-ci-test',
+  x: 'exec',
+  why: 'explain',
+  la: 'll',
+  verison: 'version',
+  ic: 'ci',
 
-const plumbing = ['help-search']
-const allCommands = [...commands, ...plumbing].sort(localeCompare)
-const abbrevs = abbrev(commands.concat(Object.keys(aliases)))
+  // typos
+  innit: 'init',
+  // manually abbrev so that install-test doesn't make insta stop working
+  in: 'install',
+  ins: 'install',
+  inst: 'install',
+  insta: 'install',
+  instal: 'install',
+  isnt: 'install',
+  isnta: 'install',
+  isntal: 'install',
+  isntall: 'install',
+  'install-clean': 'ci',
+  'isntall-clean': 'ci',
+  hlep: 'help',
+  'dist-tags': 'dist-tag',
+  upgrade: 'update',
+  udpate: 'update',
+  rum: 'run-script',
+  sit: 'install-ci-test',
+  urn: 'run-script',
+  ogr: 'org',
+  'add-user': 'adduser',
+}
 
 module.exports = {
-  abbrevs,
   aliases,
   commands,
-  plumbing,
-  allCommands,
 }

smoke-tests/tap-snapshots/test/index.js.test.cjs

@@ -24,12 +24,13 @@ All commands:
     access, adduser, audit, bugs, cache, ci, completion,
     config, dedupe, deprecate, diff, dist-tag, docs, doctor,
     edit, exec, explain, explore, find-dupes, fund, get, help,
-    hook, init, install, install-ci-test, install-test, link,
-    ll, login, logout, ls, org, outdated, owner, pack, ping,
-    pkg, prefix, profile, prune, publish, query, rebuild, repo,
-    restart, root, run-script, search, set, shrinkwrap, star,
-    stars, start, stop, team, test, token, uninstall, unpublish,
-    unstar, update, version, view, whoami
+    help-search, hook, init, install, install-ci-test,
+    install-test, link, ll, login, logout, ls, org, outdated,
+    owner, pack, ping, pkg, prefix, profile, prune, publish,
+    query, rebuild, repo, restart, root, run-script, search,
+    set, shrinkwrap, star, stars, start, stop, team, test,
+    token, uninstall, unpublish, unstar, update, version, view,
+    whoami
 
 Specify configs in the ini-formatted file:
     {NPM}/{TESTDIR}/project/.npmrc

tap-snapshots/test/lib/commands/completion.js.test.cjs

@@ -66,6 +66,7 @@ Array [
       fund
       get
       help
+      help-search
       hook
       init
       install

tap-snapshots/test/lib/docs.js.test.cjs

@@ -66,12 +66,12 @@ t.test('usage', async t => {
   // are all in sync. eg, this will error if a command is removed but not its docs file
   t.strictSame(
     fsCommands.sort(localeCompare),
-    cmdList.allCommands,
+    cmdList.commands,
     'command list and fs are the same'
   )
   t.strictSame(
     allDocs.filter(f => !bareCommands.includes(f)).sort(localeCompare),
-    cmdList.allCommands,
+    cmdList.commands,
     'command list and docs files are the same'
   )
 

tap-snapshots/test/lib/npm.js.test.cjs

@@ -5,12 +5,12 @@
 const t = require('tap')
 const util = require('util')
 const { load: loadMockNpm } = require('../fixtures/mock-npm.js')
-const { allCommands } = require('../../lib/utils/cmd-list.js')
+const { commands } = require('../../lib/utils/cmd-list.js')
 
 const isAsyncFn = (v) => typeof v === 'function' && /^\[AsyncFunction:/.test(util.inspect(v))
 
 t.test('load each command', async t => {
-  for (const cmd of allCommands) {
+  for (const cmd of commands) {
     t.test(cmd, async t => {
       const { npm, outputs, cmd: impl } = await loadMockNpm(t, {
         command: cmd,

test/lib/docs.js

@@ -561,6 +561,7 @@ t.test('aliases and typos', async t => {
   await t.resolves(npm.cmd('it'), { name: 'install-test' })
   await t.resolves(npm.cmd('installTe'), { name: 'install-test' })
   await t.resolves(npm.cmd('access'), { name: 'access' })
+  await t.resolves(npm.cmd('auth'), { name: 'owner' })
 })
 
 t.test('explicit workspace rejection', async t => {
@@ -706,40 +707,19 @@ t.test('usage', async t => {
   })
 
   t.test('set process.stdout.columns', async t => {
-    const { npm } = await loadMockNpm(t)
+    const { npm } = await loadMockNpm(t, {
+      config: { viewer: 'man' },
+    })
+    t.cleanSnapshot = str =>
+      str.replace(npm.config.get('userconfig'), '{USERCONFIG}').replace(npm.npmRoot, '{NPMROOT}')
 
-    const colUsage = async (cols) => {
-      const usages = []
-      for (const col of cols) {
-        mockGlobals(t, { 'process.stdout.columns': col })
+    const widths = [0, 1, 10, 24, 40, 41, 75, 76, 90, 100]
+    for (const width of widths) {
+      t.test(`column width ${width}`, async t => {
+        mockGlobals(t, { 'process.stdout.columns': width })
         const usage = await npm.usage
-        usages.push(usage)
-      }
-      return usages
+        t.matchSnapshot(usage)
+      })
     }
-
-    t.test('max size', async t => {
-      const usages = await colUsage([0, 76, 90, 100])
-      t.equal(usages.filter(Boolean).length, 4)
-      t.equal(new Set([...usages]).size, 1)
-    })
-
-    t.test('across max boundary', async t => {
-      const usages = await colUsage([75, 76])
-      t.equal(usages.filter(Boolean).length, 2)
-      t.equal(new Set([...usages]).size, 2)
-    })
-
-    t.test('min size', async t => {
-      const usages = await colUsage([1, 10, 24, 40])
-      t.equal(usages.filter(Boolean).length, 4)
-      t.equal(new Set([...usages]).size, 1)
-    })
-
-    t.test('different cols within min/max', async t => {
-      const usages = await colUsage([40, 41])
-      t.equal(usages.filter(Boolean).length, 2)
-      t.equal(new Set([...usages]).size, 2)
-    })
   })
 })