chore: utils cleanup and tests

- remove spawn util, refactor help command
- add tests for read-user-info, minor refactor
- add tests for pulse-till-done util, refactor
- add tests for otplease util
- add tests for open-url util
- remove unused no-progress-while-running util

PR-URL: https://github.com/npm/cli/pull/2601
Credit: @nlf
Close: #2601
Reviewed-by: @ruyadorno, @wraithgar

5 files changed, 88 insertions, 184 deletions

diff --git a/lib/help.js b/lib/help.js
index 171c52704..f69961665 100644
--- a/lib/help.js
+++ b/lib/help.js
@@ -8,22 +8,22 @@ help.completion = function (opts, cb) {
 }
 
 const npmUsage = require('./utils/npm-usage.js')
-var path = require('path')
-var spawn = require('./utils/spawn')
-var npm = require('./npm.js')
-var log = require('npmlog')
-var openUrl = require('./utils/open-url')
-var glob = require('glob')
-var output = require('./utils/output.js')
+const { spawn } = require('child_process')
+const path = require('path')
+const npm = require('./npm.js')
+const log = require('npmlog')
+const openUrl = require('./utils/open-url')
+const glob = require('glob')
+const output = require('./utils/output.js')
 
 const usage = require('./utils/usage.js')
 
 help.usage = usage('help', 'npm help <term> [<terms..>]')
 
 function help (args, cb) {
-  var argv = npm.config.parsedArgv.cooked
+  const argv = npm.config.parsedArgv.cooked
 
-  var argnum = 0
+  let argnum = 0
   if (args.length === 2 && ~~args[0])
     argnum = ~~args.shift()
 
@@ -34,7 +34,7 @@ function help (args, cb) {
   const affordances = {
     'find-dupes': 'dedupe',
   }
-  var section = affordances[args[0]] || npm.deref(args[0]) || args[0]
+  let section = affordances[args[0]] || npm.deref(args[0]) || args[0]
 
   // npm help <noargs>:  show basic usage
   if (!section) {
@@ -52,15 +52,12 @@ function help (args, cb) {
     return cb()
   }
 
-  var pref = [1, 5, 7]
-  if (argnum) {
-    pref = [argnum].concat(pref.filter(function (n) {
-      return n !== argnum
-    }))
-  }
+  let pref = [1, 5, 7]
+  if (argnum)
+    pref = [argnum].concat(pref.filter(n => n !== argnum))
 
   // npm help <section>: Try to find the path
-  var manroot = path.resolve(__dirname, '..', 'man')
+  const manroot = path.resolve(__dirname, '..', 'man')
 
   // legacy
   if (section === 'global')
@@ -71,18 +68,18 @@ function help (args, cb) {
   // find either /section.n or /npm-section.n
   // The glob is used in the glob.  The regexp is used much
   // further down.  Globs and regexps are different
-  var compextglob = '.+(gz|bz2|lzma|[FYzZ]|xz)'
-  var compextre = '\\.(gz|bz2|lzma|[FYzZ]|xz)$'
-  var f = '+(npm-' + section + '|' + section + ').[0-9]?(' + compextglob + ')'
-  return glob(manroot + '/*/' + f, function (er, mans) {
+  const compextglob = '.+(gz|bz2|lzma|[FYzZ]|xz)'
+  const compextre = '\\.(gz|bz2|lzma|[FYzZ]|xz)$'
+  const f = '+(npm-' + section + '|' + section + ').[0-9]?(' + compextglob + ')'
+  return glob(manroot + '/*/' + f, (er, mans) => {
     if (er)
       return cb(er)
 
     if (!mans.length)
       return npm.commands['help-search'](args, cb)
 
-    mans = mans.map(function (man) {
-      var ext = path.extname(man)
+    mans = mans.map((man) => {
+      const ext = path.extname(man)
       if (man.match(new RegExp(compextre)))
         man = path.basename(man, ext)
 
@@ -94,14 +91,12 @@ function help (args, cb) {
 }
 
 function pickMan (mans, pref_) {
-  var nre = /([0-9]+)$/
-  var pref = {}
-  pref_.forEach(function (sect, i) {
-    pref[sect] = i
-  })
-  mans = mans.sort(function (a, b) {
-    var an = a.match(nre)[1]
-    var bn = b.match(nre)[1]
+  const nre = /([0-9]+)$/
+  const pref = {}
+  pref_.forEach((sect, i) => pref[sect] = i)
+  mans = mans.sort((a, b) => {
+    const an = a.match(nre)[1]
+    const bn = b.match(nre)[1]
     return an === bn ? (a > b ? -1 : 1)
       : pref[an] < pref[bn] ? -1
       : 1
@@ -110,48 +105,61 @@ function pickMan (mans, pref_) {
 }
 
 function viewMan (man, cb) {
-  var nre = /([0-9]+)$/
-  var num = man.match(nre)[1]
-  var section = path.basename(man, '.' + num)
+  const nre = /([0-9]+)$/
+  const num = man.match(nre)[1]
+  const section = path.basename(man, '.' + num)
 
   // at this point, we know that the specified man page exists
-  var manpath = path.join(__dirname, '..', 'man')
-  var env = {}
+  const manpath = path.join(__dirname, '..', 'man')
+  const env = {}
   Object.keys(process.env).forEach(function (i) {
     env[i] = process.env[i]
   })
   env.MANPATH = manpath
-  var viewer = npm.config.get('viewer')
+  const viewer = npm.config.get('viewer')
+
+  const opts = {
+    env,
+    stdio: 'inherit',
+  }
 
-  var conf
+  let bin = 'man'
+  const args = []
   switch (viewer) {
     case 'woman':
-      var a = ['-e', '(woman-find-file \'' + man + '\')']
-      conf = { env: env, stdio: 'inherit' }
-      var woman = spawn('emacsclient', a, conf)
-      woman.on('close', cb)
+      bin = 'emacsclient'
+      args.push('-e', `(woman-find-file '${man}')`)
       break
 
     case 'browser':
+      bin = false
       try {
-        var url = htmlMan(man)
+        const url = htmlMan(man)
+        openUrl(url, 'help available at the following URL', cb)
       } catch (err) {
         return cb(err)
       }
-      openUrl(url, 'help available at the following URL', cb)
       break
 
     default:
-      conf = { env: env, stdio: 'inherit' }
-      var manProcess = spawn('man', [num, section], conf)
-      manProcess.on('close', cb)
+      args.push(num, section)
       break
   }
+
+  if (bin) {
+    const proc = spawn(bin, args, opts)
+    proc.on('exit', (code) => {
+      if (code)
+        return cb(new Error(`help process exited with code: ${code}`))
+
+      return cb()
+    })
+  }
 }
 
 function htmlMan (man) {
-  var sect = +man.match(/([0-9]+)$/)[1]
-  var f = path.basename(man).replace(/[.]([0-9]+)$/, '')
+  let sect = +man.match(/([0-9]+)$/)[1]
+  const f = path.basename(man).replace(/[.]([0-9]+)$/, '')
   switch (sect) {
     case 1:
       sect = 'commands'
@@ -169,7 +177,7 @@ function htmlMan (man) {
 }
 
 function getSections (cb) {
-  var g = path.resolve(__dirname, '../man/man[0-9]/*.[0-9]')
+  const g = path.resolve(__dirname, '../man/man[0-9]/*.[0-9]')
   glob(g, function (er, files) {
     if (er)
       return cb(er)
diff --git a/lib/utils/no-progress-while-running.js b/lib/utils/no-progress-while-running.js
deleted file mode 100644
index c2e6a01b2..000000000
--- a/lib/utils/no-progress-while-running.js
+++ /dev/null
@@ -1,25 +0,0 @@
-var log = require('npmlog')
-var progressEnabled
-var running = 0
-
-var startRunning = exports.startRunning = function () {
-  if (progressEnabled == null)
-    progressEnabled = log.progressEnabled
-  if (progressEnabled)
-    log.disableProgress()
-  ++running
-}
-
-var stopRunning = exports.stopRunning = function () {
-  --running
-  if (progressEnabled && running === 0)
-    log.enableProgress()
-}
-
-exports.tillDone = function noProgressTillDone (cb) {
-  startRunning()
-  return function () {
-    stopRunning()
-    cb.apply(this, arguments)
-  }
-}
diff --git a/lib/utils/pulse-till-done.js b/lib/utils/pulse-till-done.js
index 13147bae1..a88b8aacd 100644
--- a/lib/utils/pulse-till-done.js
+++ b/lib/utils/pulse-till-done.js
@@ -1,41 +1,26 @@
 const log = require('npmlog')
 
-let pulsers = 0
-let pulse
+let pulseTimer = null
+const withPromise = async (promise) => {
+  pulseStart()
+  try {
+    return await promise
+  } finally {
+    pulseStop()
+  }
+}
 
-function pulseStart (prefix) {
-  if (++pulsers > 1)
-    return
-  pulse = setInterval(function () {
-    log.gauge.pulse(prefix)
+const pulseStart = () => {
+  pulseTimer = pulseTimer || setInterval(() => {
+    log.gauge.pulse('')
   }, 150)
 }
-function pulseStop () {
-  if (--pulsers > 0)
-    return
-  clearInterval(pulse)
-}
 
-module.exports = function (prefix, cb) {
-  if (!prefix)
-    prefix = 'network'
-  pulseStart(prefix)
-  return (er, ...args) => {
-    pulseStop()
-    cb(er, ...args)
-  }
+const pulseStop = () => {
+  clearInterval(pulseTimer)
+  pulseTimer = null
 }
 
-const pulseWhile = async (prefix, promise) => {
-  if (!promise) {
-    promise = prefix
-    prefix = ''
-  }
-  pulseStart(prefix)
-  try {
-    return await promise
-  } finally {
-    pulseStop()
-  }
+module.exports = {
+  withPromise,
 }
-module.exports.withPromise = pulseWhile
diff --git a/lib/utils/read-user-info.js b/lib/utils/read-user-info.js
index b0166e18c..e3c4a9fbe 100644
--- a/lib/utils/read-user-info.js
+++ b/lib/utils/read-user-info.js
@@ -8,21 +8,21 @@ exports.password = readPassword
 exports.username = readUsername
 exports.email = readEmail
 
+const otpPrompt = `This command requires a one-time password (OTP) from your authenticator app.
+Enter one below. You can also pass one on the command line by appending --otp=123456.
+For more information, see:
+https://docs.npmjs.com/getting-started/using-two-factor-authentication
+Enter OTP: `
+const passwordPrompt = 'npm password: '
+const usernamePrompt = 'npm username: '
+const emailPrompt = 'email (this IS public): '
+
 function read (opts) {
   log.clearProgress()
   return readAsync(opts).finally(() => log.showProgress())
 }
 
-function readOTP (msg, otp, isRetry) {
-  if (!msg) {
-    msg = [
-      'This command requires a one-time password (OTP) from your authenticator app.',
-      'Enter one below. You can also pass one on the command line by appending --otp=123456.',
-      'For more information, see:',
-      'https://docs.npmjs.com/getting-started/using-two-factor-authentication',
-      'Enter OTP: ',
-    ].join('\n')
-  }
+function readOTP (msg = otpPrompt, otp, isRetry) {
   if (isRetry && otp && /^[\d ]+$|^[A-Fa-f0-9]{64,64}$/.test(otp))
     return otp.replace(/\s+/g, '')
 
@@ -30,9 +30,7 @@ function readOTP (msg, otp, isRetry) {
     .then((otp) => readOTP(msg, otp, true))
 }
 
-function readPassword (msg, password, isRetry) {
-  if (!msg)
-    msg = 'npm password: '
+function readPassword (msg = passwordPrompt, password, isRetry) {
   if (isRetry && password)
     return password
 
@@ -40,9 +38,7 @@ function readPassword (msg, password, isRetry) {
     .then((password) => readPassword(msg, password, true))
 }
 
-function readUsername (msg, username, opts, isRetry) {
-  if (!msg)
-    msg = 'npm username: '
+function readUsername (msg = usernamePrompt, username, opts = {}, isRetry) {
   if (isRetry && username) {
     const error = userValidate.username(username)
     if (error)
@@ -55,9 +51,7 @@ function readUsername (msg, username, opts, isRetry) {
     .then((username) => readUsername(msg, username, opts, true))
 }
 
-function readEmail (msg, email, opts, isRetry) {
-  if (!msg)
-    msg = 'email (this IS public): '
+function readEmail (msg = emailPrompt, email, opts = {}, isRetry) {
   if (isRetry && email) {
     const error = userValidate.email(email)
     if (error)
diff --git a/lib/utils/spawn.js b/lib/utils/spawn.js
deleted file mode 100644
index 3bbe18384..000000000
--- a/lib/utils/spawn.js
+++ /dev/null
@@ -1,58 +0,0 @@
-module.exports = spawn
-
-var _spawn = require('child_process').spawn
-var EventEmitter = require('events').EventEmitter
-var npwr = require('./no-progress-while-running.js')
-
-function willCmdOutput (stdio) {
-  if (stdio === 'inherit')
-    return true
-  if (!Array.isArray(stdio))
-    return false
-  for (var fh = 1; fh <= 2; ++fh) {
-    if (stdio[fh] === 'inherit')
-      return true
-    if (stdio[fh] === 1 || stdio[fh] === 2)
-      return true
-  }
-  return false
-}
-
-function spawn (cmd, args, options) {
-  var cmdWillOutput = willCmdOutput(options && options.stdio)
-
-  if (cmdWillOutput)
-    npwr.startRunning()
-  var raw = _spawn(cmd, args, options)
-  var cooked = new EventEmitter()
-
-  raw.on('error', function (er) {
-    if (cmdWillOutput)
-      npwr.stopRunning()
-    er.file = cmd
-    cooked.emit('error', er)
-  }).on('close', function (code, signal) {
-    if (cmdWillOutput)
-      npwr.stopRunning()
-    // Create ENOENT error because Node.js v0.8 will not emit
-    // an `error` event if the command could not be found.
-    if (code === 127) {
-      var er = new Error('spawn ENOENT')
-      er.code = 'ENOENT'
-      er.errno = 'ENOENT'
-      er.syscall = 'spawn'
-      er.file = cmd
-      cooked.emit('error', er)
-    } else
-      cooked.emit('close', code, signal)
-  })
-
-  cooked.stdin = raw.stdin
-  cooked.stdout = raw.stdout
-  cooked.stderr = raw.stderr
-  cooked.kill = function (sig) {
-    return raw.kill(sig)
-  }
-
-  return cooked
-}