chore(refactor): async npm.load

Tests for cli now use the real npm

PR-URL: #3451
Credit: @wraithgar
Close: #3451
Reviewed-by: @nlf

lib/cli.js

@@ -1,5 +1,5 @@
 // Separated out for easier unit testing
-module.exports = (process) => {
+module.exports = async (process) => {
   // set it here so that regardless of what happens later, we don't
   // leak any private CLI configs to other programs
   process.title = 'npm'
@@ -39,12 +39,8 @@ module.exports = (process) => {
 
   // now actually fire up npm and run the command.
   // this is how to use npm programmatically:
-  npm.load(async er => {
-    // Any exceptions here will be picked up by the uncaughtException handler
-    if (er)
-      return exitHandler(er)
-
-    // npm --version=cli
+  try {
+    await npm.load()
     if (npm.config.get('version', 'cli')) {
       npm.output(npm.version)
       return exitHandler()
@@ -75,5 +71,7 @@ module.exports = (process) => {
     }
 
     impl(npm.argv, exitHandler)
-  })
+  } catch (err) {
+    return exitHandler(err)
+  }
 }

lib/npm.js

@@ -11,6 +11,7 @@ const Config = require('@npmcli/config')
 // Patch the global fs module here at the app level
 require('graceful-fs').gracefulify(require('fs'))
 
+// TODO make this only ever load once (or unload) in tests
 const procLogListener = require('./utils/proc-log-listener.js')
 
 const proxyCmds = new Proxy({}, {
@@ -48,6 +49,7 @@ const _title = Symbol('_title')
 const npm = module.exports = new class extends EventEmitter {
   constructor () {
     super()
+    // TODO make this only ever load once (or unload) in tests
     require('./utils/perf.js')
     this.started = Date.now()
     this.command = null
@@ -77,8 +79,8 @@ const npm = module.exports = new class extends EventEmitter {
   [_runCmd] (cmd, impl, args, cb) {
     if (!this.loaded) {
       throw new Error(
-        'Call npm.load(cb) before using this command.\n' +
-        'See the README.md or bin/npm-cli.js for example usage.'
+        'Call npm.load() before using this command.\n' +
+        'See lib/cli.js for example usage.'
       )
     }
 
@@ -96,7 +98,7 @@ const npm = module.exports = new class extends EventEmitter {
       args.filter(arg => /^[\u2010-\u2015\u2212\uFE58\uFE63\uFF0D]/.test(arg))
         .forEach(arg => {
           warnedNonDashArg = true
-          log.error('arg', 'Argument starts with non-ascii dash, this is probably invalid:', arg)
+          this.log.error('arg', 'Argument starts with non-ascii dash, this is probably invalid:', arg)
         })
     }
 
@@ -123,33 +125,32 @@ const npm = module.exports = new class extends EventEmitter {
     }
   }
 
-  // call with parsed CLI options and a callback when done loading
-  // XXX promisify this and stop taking a callback
   load (cb) {
-    if (!cb || typeof cb !== 'function')
-      throw new TypeError('must call as: npm.load(callback)')
-
-    this.once('load', cb)
-    if (this.loaded || this.loadErr) {
-      this.emit('load', this.loadErr)
-      return
+    if (cb && typeof cb !== 'function')
+      throw new TypeError('callback must be a function if provided')
+
+    if (!this.loadPromise) {
+      process.emit('time', 'npm:load')
+      this.log.pause()
+      this.loadPromise = new Promise((resolve, reject) => {
+        this[_load]().catch(er => er).then((er) => {
+          this.loadErr = er
+          if (!er && this.config.get('force'))
+            this.log.warn('using --force', 'Recommended protections disabled.')
+
+          process.emit('timeEnd', 'npm:load')
+          if (er)
+            return reject(er)
+          resolve()
+        })
+      })
     }
-    if (this.loading)
-      return
-
-    this.loading = true
+    if (!cb)
+      return this.loadPromise
 
-    process.emit('time', 'npm:load')
-    this.log.pause()
-    return this[_load]().catch(er => er).then((er) => {
-      this.loading = false
-      this.loadErr = er
-      if (!er && this.config.get('force'))
-        this.log.warn('using --force', 'Recommended protections disabled.')
-
-      process.emit('timeEnd', 'npm:load')
-      this.emit('load', er)
-    })
+    // loadPromise is returned here for legacy purposes, old code was allowing
+    // the mixing of callback and promise here.
+    return this.loadPromise.then(cb, cb)
   }
 
   get loaded () {
@@ -167,10 +168,15 @@ const npm = module.exports = new class extends EventEmitter {
 
   async [_load] () {
     process.emit('time', 'npm:load:whichnode')
-    const node = await which(process.argv[0]).catch(er => null)
+    let node
+    try {
+      node = which.sync(process.argv[0])
+    } catch (_) {
+      // TODO should we throw here?
+    }
     process.emit('timeEnd', 'npm:load:whichnode')
     if (node && node.toUpperCase() !== process.execPath.toUpperCase()) {
-      log.verbose('node symlink', node)
+      this.log.verbose('node symlink', node)
       process.execPath = node
       this.config.execPath = node
     }
@@ -198,10 +204,10 @@ const npm = module.exports = new class extends EventEmitter {
     process.env.COLOR = this.color ? '1' : '0'
 
     process.emit('time', 'npm:load:cleanupLog')
-    cleanUpLogFiles(this.cache, this.config.get('logs-max'), log.warn)
+    cleanUpLogFiles(this.cache, this.config.get('logs-max'), this.log.warn)
     process.emit('timeEnd', 'npm:load:cleanupLog')
 
-    log.resume()
+    this.log.resume()
 
     process.emit('time', 'npm:load:configScope')
     const configScope = this.config.get('scope')
@@ -314,9 +320,8 @@ const npm = module.exports = new class extends EventEmitter {
 // now load everything required by the class methods
 
 const log = require('npmlog')
-const { promisify } = require('util')
 
-const which = promisify(require('which'))
+const which = require('which')
 
 const deref = require('./utils/deref-command.js')
 const setupLog = require('./utils/setup-log.js')

lib/utils/perf.js

@@ -14,3 +14,10 @@ process.on('timeEnd', (name) => {
   } else
     log.silly('timing', "Tried to end timer that doesn't exist:", name)
 })
+
+// for tests
+/* istanbul ignore next */
+exports.reset = () => {
+  process.removeAllListeners('time')
+  process.removeAllListeners('timeEnd')
+}

lib/utils/proc-log-listener.js

@@ -14,3 +14,9 @@ module.exports = () => {
     }
   })
 }
+
+// for tests
+/* istanbul ignore next */
+module.exports.reset = () => {
+  process.removeAllListeners('log')
+}