From a5cfd86a4954811ad44729097babf0451e2a3bce Mon Sep 17 00:00:00 2001 From: bcoe Date: Sun, 25 Aug 2019 20:59:09 -0700 Subject: [PATCH 1/3] fix: populate correct type for yargs.parsed --- index.js | 1 - lib/command.js | 17 ++++++---- lib/usage.js | 8 +++++ test/command.js | 82 +++++++++++++++++++++++++++++++++---------------- yargs.js | 6 ++-- 5 files changed, 79 insertions(+), 35 deletions(-) diff --git a/index.js b/index.js index b36531b43..2db543ed3 100644 --- a/index.js +++ b/index.js @@ -32,7 +32,6 @@ function singletonify (inst) { return inst.$0 }) Argv.__defineGetter__('parsed', () => { - console.warn('In future major releases of yargs, "parsed" will be a private field. Use the return value of ".parse()" or ".argv" instead') return inst.parsed }) } diff --git a/lib/command.js b/lib/command.js index 702431053..ed5cdaa7e 100644 --- a/lib/command.js +++ b/lib/command.js @@ -174,7 +174,7 @@ module.exports = function command (yargs, usage, validation, globalMiddleware) { let numFiles = currentContext.files.length const parentCommands = currentContext.commands.slice() - // what does yargs look like after the buidler is run? + // what does yargs look like after the builder is run? let innerArgv = parsed.argv let innerYargs = null let positionalMap = {} @@ -230,14 +230,19 @@ module.exports = function command (yargs, usage, validation, globalMiddleware) { innerArgv = applyMiddleware(innerArgv, yargs, middlewares, false) - const handlerResult = isPromise(innerArgv) - ? innerArgv.then(argv => commandHandler.handler(argv)) - : commandHandler.handler(innerArgv) + let handlerResult + if (isPromise(innerArgv)) { + yargs.getUsageInstance().cacheHelpMessage() + handlerResult = innerArgv.then(argv => commandHandler.handler(argv)) + } else { + handlerResult = commandHandler.handler(innerArgv) + } if (isPromise(handlerResult)) { - handlerResult.catch(error => + yargs.getUsageInstance().cacheHelpMessage() + handlerResult.catch(error => { yargs.getUsageInstance().fail(null, error) - ) + }) } } diff --git a/lib/usage.js b/lib/usage.js index fed6d9f69..92bf37862 100644 --- a/lib/usage.js +++ b/lib/usage.js @@ -148,6 +148,7 @@ module.exports = function usage (yargs, y18n) { const defaultGroup = 'Options:' self.help = function help () { + if (cachedHelpMessage) return cachedHelpMessage normalizeAliases() // handle old demanded API @@ -403,6 +404,13 @@ module.exports = function usage (yargs, y18n) { }) } + // if yargs is executing an async handler, we take a snapshot of the + // help message to display on failure: + let cachedHelpMessage + self.cacheHelpMessage = function () { + cachedHelpMessage = this.help() + } + // given a set of keys, place any keys that are // ungrouped under the 'Options:' grouping. function addUngroupedKeys (keys, aliases, groups) { diff --git a/test/command.js b/test/command.js index 9c394ce61..f4b4629d3 100644 --- a/test/command.js +++ b/test/command.js @@ -1426,36 +1426,66 @@ describe('Command', () => { }) }) - // addresses https://github.com/yargs/yargs/issues/510 - it('fails when the promise returned by the command handler rejects', (done) => { - const error = new Error() - yargs('foo') - .command('foo', 'foo command', noop, (yargs) => Promise.reject(error)) - .fail((msg, err) => { - expect(msg).to.equal(null) - expect(err).to.equal(error) - done() + describe('async', () => { + // addresses https://github.com/yargs/yargs/issues/510 + it('fails when the promise returned by the command handler rejects', (done) => { + const error = new Error() + yargs('foo') + .command('foo', 'foo command', noop, (yargs) => Promise.reject(error)) + .fail((msg, err) => { + expect(msg).to.equal(null) + expect(err).to.equal(error) + done() + }) + .parse() + }) + + it('succeeds when the promise returned by the command handler resolves', (done) => { + const handler = new Promise((resolve, reject) => { + setTimeout(() => { + return resolve(true) + }, 5) }) - .parse() - }) + const parsed = yargs('foo hello') + .command('foo ', 'foo command', () => {}, (yargs) => handler) + .fail((msg, err) => { + return done(Error('should not have been called')) + }) + .parse() - it('succeeds when the promise returned by the command handler resolves', (done) => { - const handler = new Promise((resolve, reject) => { - setTimeout(() => { - return resolve(true) - }, 5) - }) - const parsed = yargs('foo hello') - .command('foo ', 'foo command', () => {}, (yargs) => handler) - .fail((msg, err) => { - return done(Error('should not have been called')) + handler.then(called => { + called.should.equal(true) + parsed.pos.should.equal('hello') + return done() }) - .parse() + }) - handler.then(called => { - called.should.equal(true) - parsed.pos.should.equal('hello') - return done() + // see: https://github.com/yargs/yargs/issues/1144 + it('displays error and appropriate help message when handler fails', (done) => { + // TODO: debug why exception bubbles to unhandledRejection + // if we set exitProcess to false; currently we are taking + // advantage of this to determine when to call done(). + let errorLog = '' + const check = (err) => { + process.removeListener('unhandledRejection', check) + err.message.should.include('foo error') + errorLog.should.include('foo command') + return done() + } + process.on('unhandledRejection', check) + const y = yargs('foo') + .command('foo', 'foo command', () => {}, (argv) => { + return Promise.reject(Error('foo error')) + }) + .exitProcess(false) + // the bug reported in #1144 only happens when + // usage.help() is called, this does not occur when + // a failure handler is attached. We override the logger + // so that we can catch console output: + y._getLoggerInstance().error = (out) => { + errorLog += out + } + y.parse() }) }) diff --git a/yargs.js b/yargs.js index 5b23a0f87..794cf0255 100644 --- a/yargs.js +++ b/yargs.js @@ -544,10 +544,12 @@ function Yargs (processArgs, cwd, parentRequire) { argsert('[string|array] [function|boolean|object] [function]', [args, shortCircuit, _parseFn], arguments.length) freeze() if (typeof args === 'undefined') { - const parsed = self._parseArgs(processArgs) + const argv = self._parseArgs(processArgs) + const tmpParsed = self.parsed unfreeze() // TODO: remove this compatibility hack when we release yargs@15.x: - return (this.parsed = parsed) + self.parsed = tmpParsed + return argv } // a context object can optionally be provided, this allows From 63b51c74971bc640b03dbb4df2649d11708c208f Mon Sep 17 00:00:00 2001 From: bcoe Date: Thu, 29 Aug 2019 21:51:32 -0700 Subject: [PATCH 2/3] chore: address code review --- lib/command.js | 6 +++++- test/command.js | 33 ++++++++++++++++++--------------- 2 files changed, 23 insertions(+), 16 deletions(-) diff --git a/lib/command.js b/lib/command.js index ed5cdaa7e..0901c8e04 100644 --- a/lib/command.js +++ b/lib/command.js @@ -241,7 +241,11 @@ module.exports = function command (yargs, usage, validation, globalMiddleware) { if (isPromise(handlerResult)) { yargs.getUsageInstance().cacheHelpMessage() handlerResult.catch(error => { - yargs.getUsageInstance().fail(null, error) + try { + yargs.getUsageInstance().fail(null, error) + } catch (err) { + // fail's throwing would cause an unhandled rejection. + } }) } } diff --git a/test/command.js b/test/command.js index f4b4629d3..d3a5628fd 100644 --- a/test/command.js +++ b/test/command.js @@ -1462,30 +1462,33 @@ describe('Command', () => { // see: https://github.com/yargs/yargs/issues/1144 it('displays error and appropriate help message when handler fails', (done) => { - // TODO: debug why exception bubbles to unhandledRejection - // if we set exitProcess to false; currently we are taking - // advantage of this to determine when to call done(). - let errorLog = '' - const check = (err) => { - process.removeListener('unhandledRejection', check) - err.message.should.include('foo error') - errorLog.should.include('foo command') - return done() - } - process.on('unhandledRejection', check) const y = yargs('foo') - .command('foo', 'foo command', () => {}, (argv) => { + .command('foo', 'foo command', (yargs) => { + yargs.option('bar', { + describe: 'bar option' + }) + }, (argv) => { return Promise.reject(Error('foo error')) }) .exitProcess(false) // the bug reported in #1144 only happens when // usage.help() is called, this does not occur when - // a failure handler is attached. We override the logger - // so that we can catch console output: + // console output is suppressed. tldr; we capture + // the log output: + let errorLog = '' y._getLoggerInstance().error = (out) => { - errorLog += out + if (out) errorLog += `${out}\n` } y.parse() + // the promise returned by the handler rejects immediately, + // so we can wait for a small number of ms: + setTimeout(() => { + // ensure the appropriate help is displayed: + errorLog.should.include('bar option') + // ensure error was displayed: + errorLog.should.include('foo error') + return done() + }, 5) }) }) From baa2da799b4bfe84145e3b8d45e0c872b0516040 Mon Sep 17 00:00:00 2001 From: bcoe Date: Thu, 29 Aug 2019 21:52:44 -0700 Subject: [PATCH 3/3] chore: address code review --- lib/command.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/command.js b/lib/command.js index 0901c8e04..02bc7eac3 100644 --- a/lib/command.js +++ b/lib/command.js @@ -232,7 +232,6 @@ module.exports = function command (yargs, usage, validation, globalMiddleware) { let handlerResult if (isPromise(innerArgv)) { - yargs.getUsageInstance().cacheHelpMessage() handlerResult = innerArgv.then(argv => commandHandler.handler(argv)) } else { handlerResult = commandHandler.handler(innerArgv)