Skip to content

Commit

Permalink
fix: run update notifier after exec but before waiting
Browse files Browse the repository at this point in the history
  • Loading branch information
lukekarrys committed Apr 24, 2024
1 parent f309c1c commit e5f1948
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 28 deletions.
29 changes: 16 additions & 13 deletions lib/cli/entry.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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))
Expand All @@ -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()
Expand Down
13 changes: 7 additions & 6 deletions lib/npm.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`)
Expand Down Expand Up @@ -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 () {
Expand Down
16 changes: 7 additions & 9 deletions lib/utils/did-you-mean.js
Original file line number Diff line number Diff line change
Expand Up @@ -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']
Expand All @@ -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

0 comments on commit e5f1948

Please sign in to comment.