fix: run update notifier after exec but before waiting

Author: lukekarrys

lib/cli/entry.js

@@ -36,7 +36,6 @@ module.exports = async (process, validateEngines) => {
     log.warn('cli', validateEngines.unsupportedMessage)
   }
 
-  let cmd
   // Now actually fire up npm and run the command.
   // This is how to use npm programmatically:
   try {
@@ -46,20 +45,15 @@ module.exports = async (process, validateEngines) => {
       return exitHandler()
     }
 
-    cmd = npm.argv.shift()
-    if (!cmd) {
+    const command = npm.argv.shift()
+    const args = npm.argv
+
+    if (!command) {
       output.standard(npm.usage)
       process.exitCode = 1
       return exitHandler()
     }
 
-    // this is async but we dont await it, since its ok if it doesnt
-    // finish before the command finishes running. it uses command and argv
-    // so it must be initiated here, after the command name is set
-    const updateNotifier = require('./update-notifier.js')
-    // eslint-disable-next-line promise/catch-or-return
-    updateNotifier(npm).then((msg) => (npm.updateNotification = msg))
-
     // Options are prefixed by a hyphen-minus (-, \u2d).
     // Other dash-type chars look similar but are invalid.
     const nonDashArgs = npm.argv.filter(a => /^[\u2010-\u2015\u2212\uFE58\uFE63\uFF0D]/.test(a))
@@ -71,13 +65,22 @@ module.exports = async (process, validateEngines) => {
       )
     }
 
-    await npm.exec(cmd)
+    const execPromise = npm.exec(command, args)
+
+    // this is async but we dont await it, since its ok if it doesnt
+    // finish before the command finishes running. it uses command and argv
+    // so it must be initiated here, after the command name is set
+    const updateNotifier = require('./update-notifier.js')
+    // eslint-disable-next-line promise/catch-or-return
+    updateNotifier(npm).then((msg) => (npm.updateNotification = msg))
+
+    await execPromise
     return exitHandler()
   } catch (err) {
     if (err.code === 'EUNKNOWNCOMMAND') {
       const didYouMean = require('../utils/did-you-mean.js')
-      const suggestions = await didYouMean(npm.localPrefix, cmd)
-      output.standard(`Unknown command: "${cmd}"${suggestions}\n`)
+      const suggestions = await didYouMean(npm.localPrefix, err.command)
+      output.standard(`Unknown command: "${err.command}"${suggestions}\n`)
       output.standard('To see a list of supported npm commands, run:\n  npm help')
       process.exitCode = 1
       return exitHandler()

lib/npm.js

@@ -22,6 +22,7 @@ class Npm {
     if (!command) {
       throw Object.assign(new Error(`Unknown command ${c}`), {
         code: 'EUNKNOWNCOMMAND',
+        command: c,
       })
     }
     return require(`./commands/${command}.js`)
@@ -90,18 +91,18 @@ class Npm {
   }
 
   // Call an npm command
-  // TODO: tests are currently the only time the second
-  // parameter of args is used. When called via `lib/cli.js` the config is
-  // loaded and this.argv is set to the remaining command line args. We should
-  // consider testing the CLI the same way it is used and not allow args to be
-  // passed in directly.
   async exec (cmd, args = this.argv) {
     const command = this.setCmd(cmd)
     return time.start(`command:${cmd}`, () => command.cmdExec(args))
   }
 
   async load () {
-    return time.start('npm:load', () => this.#load().then(r => r || { exec: true }))
+    return time.start('npm:load', async () => {
+      const { exec = true } = await this.#load().then(r => r ?? {})
+      return {
+        exec,
+      }
+    })
   }
 
   get loaded () {

lib/utils/did-you-mean.js

@@ -8,7 +8,7 @@ const didYouMean = async (path, scmd) => {
   let best = []
   for (const str of close) {
     const cmd = Npm.cmd(str)
-    best.push(`    npm ${str} # ${cmd.description}`)
+    best.push(`  npm ${str} # ${cmd.description}`)
   }
   // We would already be suggesting this in `npm x` so omit them here
   const runScripts = ['stop', 'start', 'test', 'restart']
@@ -17,25 +17,23 @@ const didYouMean = async (path, scmd) => {
     best = best.concat(
       Object.keys(scripts || {})
         .filter(cmd => distance(scmd, cmd) < scmd.length * 0.4 && !runScripts.includes(cmd))
-        .map(str => `    npm run ${str} # run the "${str}" package script`),
+        .map(str => `  npm run ${str} # run the "${str}" package script`),
       Object.keys(bin || {})
         .filter(cmd => distance(scmd, cmd) < scmd.length * 0.4)
         /* eslint-disable-next-line max-len */
-        .map(str => `    npm exec ${str} # run the "${str}" command from either this or a remote npm package`)
+        .map(str => `  npm exec ${str} # run the "${str}" command from either this or a remote npm package`)
     )
-  } catch (_) {
+  } catch {
     // gracefully ignore not being in a folder w/ a package.json
   }
 
   if (best.length === 0) {
     return ''
   }
 
-  const suggestion =
-    best.length === 1
-      ? `\n\nDid you mean this?\n${best[0]}`
-      : `\n\nDid you mean one of these?\n${best.slice(0, 3).join('\n')}`
-  return suggestion
+  return best.length === 1
+    ? `\n\nDid you mean this?\n${best[0]}`
+    : `\n\nDid you mean one of these?\n${best.slice(0, 3).join('\n')}`
 }
 
 module.exports = didYouMean