feat: discrete npm doctor commands (#5888)

* fix: output doctor checks as they finish
* feat: allow for discrete npm doctor checks
* feat: add environment check to npm doctor
* chore: refactor command

Author: wraithgar

docs/lib/content/commands/npm-doctor.md

@@ -31,8 +31,10 @@ Also, in addition to this, there are also very many issue reports due to
 using old versions of npm. Since npm is constantly improving, running
 `npm@latest` is better than an old version.
 
-`npm doctor` verifies the following items in your environment, and if there
-are any recommended changes, it will display them.
+`npm doctor` verifies the following items in your environment, and if
+there are any recommended changes, it will display them.  By default npm
+runs all of these checks. You can limit what checks are ran by
+specifying them as extra arguments.
 
 #### `npm ping`
 

lib/commands/doctor.js

@@ -1,14 +1,13 @@
 const cacache = require('cacache')
 const fs = require('fs')
 const fetch = require('make-fetch-happen')
-const table = require('text-table')
+const Table = require('cli-table3')
 const which = require('which')
 const pacote = require('pacote')
 const { resolve } = require('path')
 const semver = require('semver')
 const { promisify } = require('util')
 const log = require('../utils/log-shim.js')
-const ansiTrim = require('../utils/ansi-trim.js')
 const ping = require('../utils/ping.js')
 const {
   registry: { default: defaultRegistry },
@@ -17,6 +16,7 @@ const lstat = promisify(fs.lstat)
 const readdir = promisify(fs.readdir)
 const access = promisify(fs.access)
 const { R_OK, W_OK, X_OK } = fs.constants
+
 const maskLabel = mask => {
   const label = []
   if (mask & R_OK) {
@@ -34,76 +34,105 @@ const maskLabel = mask => {
   return label.join(', ')
 }
 
+const subcommands = [
+  {
+    groups: ['ping', 'registry'],
+    title: 'npm ping',
+    cmd: 'checkPing',
+  }, {
+    groups: ['versions'],
+    title: 'npm -v',
+    cmd: 'getLatestNpmVersion',
+  }, {
+    groups: ['versions'],
+    title: 'node -v',
+    cmd: 'getLatestNodejsVersion',
+  }, {
+    groups: ['registry'],
+    title: 'npm config get registry',
+    cmd: 'checkNpmRegistry',
+  }, {
+    groups: ['environment'],
+    title: 'git executable in PATH',
+    cmd: 'getGitPath',
+  }, {
+    groups: ['environment'],
+    title: 'global bin folder in PATH',
+    cmd: 'getBinPath',
+  }, {
+    groups: ['permissions', 'cache'],
+    title: 'Perms check on cached files',
+    cmd: 'checkCachePermission',
+    windows: false,
+  }, {
+    groups: ['permissions'],
+    title: 'Perms check on local node_modules',
+    cmd: 'checkLocalModulesPermission',
+    windows: false,
+  }, {
+    groups: ['permissions'],
+    title: 'Perms check on global node_modules',
+    cmd: 'checkGlobalModulesPermission',
+    windows: false,
+  }, {
+    groups: ['permissions'],
+    title: 'Perms check on local bin folder',
+    cmd: 'checkLocalBinPermission',
+    windows: false,
+  }, {
+    groups: ['permissions'],
+    title: 'Perms check on global bin folder',
+    cmd: 'checkGlobalBinPermission',
+    windows: false,
+  }, {
+    groups: ['cache'],
+    title: 'Verify cache contents',
+    cmd: 'verifyCachedFiles',
+    windows: false,
+  },
+  // TODO:
+  // group === 'dependencies'?
+  //   - ensure arborist.loadActual() runs without errors and no invalid edges
+  //   - ensure package-lock.json matches loadActual()
+  //   - verify loadActual without hidden lock file matches hidden lockfile
+  // group === '???'
+  //   - verify all local packages have bins linked
+  // What is the fix for these?
+]
 const BaseCommand = require('../base-command.js')
 class Doctor extends BaseCommand {
   static description = 'Check your npm environment'
   static name = 'doctor'
   static params = ['registry']
   static ignoreImplicitWorkspace = false
+  static usage = [`[${subcommands.flatMap(s => s.groups)
+    .filter((value, index, self) => self.indexOf(value) === index)
+    .join('] [')}]`]
+
+  static subcommands = subcommands
+
+  // minimum width of check column, enough for the word `Check`
+  #checkWidth = 5
 
   async exec (args) {
     log.info('Running checkup')
+    let allOk = true
 
-    // each message is [title, ok, message]
-    const messages = []
-
-    const actions = [
-      ['npm ping', 'checkPing', []],
-      ['npm -v', 'getLatestNpmVersion', []],
-      ['node -v', 'getLatestNodejsVersion', []],
-      ['npm config get registry', 'checkNpmRegistry', []],
-      ['which git', 'getGitPath', []],
-      ...(process.platform === 'win32'
-        ? []
-        : [
-          [
-            'Perms check on cached files',
-            'checkFilesPermission',
-            [this.npm.cache, true, R_OK],
-          ], [
-            'Perms check on local node_modules',
-            'checkFilesPermission',
-            [this.npm.localDir, true, R_OK | W_OK, true],
-          ], [
-            'Perms check on global node_modules',
-            'checkFilesPermission',
-            [this.npm.globalDir, false, R_OK],
-          ], [
-            'Perms check on local bin folder',
-            'checkFilesPermission',
-            [this.npm.localBin, false, R_OK | W_OK | X_OK, true],
-          ], [
-            'Perms check on global bin folder',
-            'checkFilesPermission',
-            [this.npm.globalBin, false, X_OK],
-          ],
-        ]),
-      [
-        'Verify cache contents',
-        'verifyCachedFiles',
-        [this.npm.flatOptions.cache],
-      ],
-      // TODO:
-      // - ensure arborist.loadActual() runs without errors and no invalid edges
-      // - ensure package-lock.json matches loadActual()
-      // - verify loadActual without hidden lock file matches hidden lockfile
-      // - verify all local packages have bins linked
-    ]
+    const actions = this.actions(args)
+    this.#checkWidth = actions.reduce((length, item) =>
+      Math.max(item.title.length, length), this.#checkWidth)
 
+    if (!this.npm.silent) {
+      this.output(['Check', 'Value', 'Recommendation/Notes'].map(h => this.npm.chalk.underline(h)))
+    }
     // Do the actual work
-    for (const [msg, fn, args] of actions) {
-      const line = [msg]
+    for (const { title, cmd } of actions) {
+      const item = [title]
       try {
-        line.push(true, await this[fn](...args))
-      } catch (er) {
-        line.push(false, er)
+        item.push(true, await this[cmd]())
+      } catch (err) {
+        item.push(false, err)
       }
-      messages.push(line)
-    }
-
-    const outHead = ['Check', 'Value', 'Recommendation/Notes'].map(h => this.npm.chalk.underline(h))
-    let allOk = true
-    const outBody = messages.map(item => {
       if (!item[1]) {
         allOk = false
         item[0] = this.npm.chalk.red(item[0])
@@ -112,18 +141,18 @@ class Doctor extends BaseCommand {
       } else {
         item[1] = this.npm.chalk.green('ok')
       }
-      return item
-    })
-    const outTable = [outHead, ...outBody]
-    const tableOpts = {
-      stringLength: s => ansiTrim(s).length,
+      if (!this.npm.silent) {
+        this.output(item)
+      }
     }
 
-    if (!this.npm.silent) {
-      this.npm.output(table(outTable, tableOpts))
-    }
     if (!allOk) {
-      throw new Error('Some problems found. See above for recommendations.')
+      if (this.npm.silent) {
+        /* eslint-disable-next-line max-len */
+        throw new Error('Some problems found. Check logs or disable silent mode for recommendations.')
+      } else {
+        throw new Error('Some problems found. See above for recommendations.')
+      }
     }
   }
 
@@ -191,6 +220,35 @@ class Doctor extends BaseCommand {
     }
   }
 
+  async getBinPath (dir) {
+    const tracker = log.newItem('getBinPath', 1)
+    tracker.info('getBinPath', 'Finding npm global bin in your PATH')
+    if (!process.env.PATH.includes(this.npm.globalBin)) {
+      throw new Error(`Add ${this.npm.globalBin} to your $PATH`)
+    }
+    return this.npm.globalBin
+  }
+
+  async checkCachePermission () {
+    return this.checkFilesPermission(this.npm.cache, true, R_OK)
+  }
+
+  async checkLocalModulesPermission () {
+    return this.checkFilesPermission(this.npm.localDir, true, R_OK | W_OK, true)
+  }
+
+  async checkGlobalModulesPermission () {
+    return this.checkFilesPermission(this.npm.globalDir, false, R_OK)
+  }
+
+  async checkLocalBinPermission () {
+    return this.checkFilesPermission(this.npm.localBin, false, R_OK | W_OK | X_OK, true)
+  }
+
+  async checkGlobalBinPermission () {
+    return this.checkFilesPermission(this.npm.globalBin, false, X_OK)
+  }
+
   async checkFilesPermission (root, shouldOwn, mask, missingOk) {
     let ok = true
 
@@ -264,7 +322,7 @@ class Doctor extends BaseCommand {
     try {
       return await which('git').catch(er => {
         tracker.warn(er)
-        throw "Install git and ensure it's in your PATH."
+        throw new Error("Install git and ensure it's in your PATH.")
       })
     } finally {
       tracker.finish()
@@ -312,6 +370,42 @@ class Doctor extends BaseCommand {
       return `using default registry (${defaultRegistry})`
     }
   }
+
+  output (row) {
+    const t = new Table({
+      chars: { top: '',
+        'top-mid': '',
+        'top-left': '',
+        'top-right': '',
+        bottom: '',
+        'bottom-mid': '',
+        'bottom-left': '',
+        'bottom-right': '',
+        left: '',
+        'left-mid': '',
+        mid: '',
+        'mid-mid': '',
+        right: '',
+        'right-mid': '',
+        middle: '  ' },
+      style: { 'padding-left': 0, 'padding-right': 0 },
+      colWidths: [this.#checkWidth, 6],
+    })
+    t.push(row)
+    this.npm.output(t.toString())
+  }
+
+  actions (params) {
+    return this.constructor.subcommands.filter(subcmd => {
+      if (process.platform === 'win32' && subcmd.windows === false) {
+        return false
+      }
+      if (params.length) {
+        return params.some(param => subcmd.groups.includes(param))
+      }
+      return true
+    })
+  }
 }
 
 module.exports = Doctor