Repository: yargs/yargs
Pull request #1412 (2019-08-26 04:06:06)

fix: populate correct value on yargs.parsed and stop warning on access
This should address a couple minor issues that lead to #1144:

1. `self.parsed` was being populated with an `argv` instance rather than a `yargsParser.detailed` instance, leading to the missing fields that raised exceptions.
2. once `1.` was addressed, the help message was incorrect, because `async` handlers prior to this relied on state not being reset; to solve this, we now cache the help message that _would_ be displayed at the time of a promise's invocation.
3. there was one final issue related to our `self.parsed`, TypeScript executes all methods on a class, making our warning message display even though it wasn't explicitly invoked (I've reverted the warning message).

*TypeScript Issue:*

```js
var __importStar = (this && this.__importStar) || function (mod) {
    if (mod && mod.__esModule) return mod;
    var result = {};
    if (mod != null) for (var k in mod) if (Object.hasOwnProperty.call(mod, k)) result[k] = mod[k];
    result["default"] = mod;
    return result;
};
```

@gajus could I get you to try this out?

fixes: #1144 

gajus: Can confirm that this fixes the issue I have reported originally.
gajus: > or having runCommand and all depending functions become async too, which would break the ability to chain calls after such methods

What is the impact of breaking ability to chain methods?

The last suggestion sounds reasonable. First one sounds like a quick workaround that will leave up to the individual yargs consumer to figure out their own way of handling async errors.
mleguen: @bcoe IMO, we have a conception issue here: we should not accept async handlers in sync chained functions.

I recommend:
- either deprecating the use of async command handlers, and ignoring command handler results, without trying to display help (which would be consistent with what happen when a sync handler throws: no help is displayed)
- or having runCommand and all depending functions become async too, which would break the ability to chain calls after such methods
mleguen: This conception issue is what leads to your last test being so difficult to write.

Any client program using async handlers and wanting to perform actions after yargs is done parsing will have the same problem: we cannot know when yargs is done parsing when using async handler.
mleguen: > What is the impact of breaking ability to chain methods?

In fact, I think it would only prevent chaining `showHelp()`, as `parse()` is already not chainable, and I cannot see any use case for chaining `showHelp()`.

However, the major impact would be that, when using async handlers, parse would no longer return directly the parsing results, but a promise. This would be great for people wanting to do things after parsing, but could break existing code.

Next major release?
mleguen: In order not to block this fix, here is what I suggest:
- fix the unhandled rejection issue
- remove the last test, as not being writable yet
- merge this PR
- open a new issue about `parse()` and `showHelp()` having to return a promise when a command handler does so
- write a PR for this new issue, including the missing test (I can handle this)

@bcoe what is your opinion on this?

Patch:
diff --git a/docs/api.md b/docs/api.md
index 5912252..9b88291 100644
--- a/docs/api.md
+++ b/docs/api.md
@@ -137,10 +137,10 @@ var argv = require('yargs')
 Provide a synchronous function to coerce or transform the value(s) given on the
 command line for `key`.
 
-The coercion function should accept one argument, representing the parsed value
-from the command line, and should return a new value or throw an error. The
-returned value will be used as the value for `key` (or one of its aliases) in
-`argv`.
+The coercion function should accept one argument, representing the parsed value from
+the command line (an array if multiple values are parsed for the key), and should 
+return a new value or throw an error. The returned value will be used as the value for
+`key` (or one of its aliases) in `argv`.
 
 If the function throws, the error will be treated as a validation
 failure, delegating to either a custom [`.fail()`](#fail) handler or printing
diff --git a/index.js b/index.js
index b36531b..2db543e 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 7024310..02bc7ea 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,22 @@ 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)) {
+        handlerResult = innerArgv.then(argv => commandHandler.handler(argv))
+      } else {
+        handlerResult = commandHandler.handler(innerArgv)
+      }
 
       if (isPromise(handlerResult)) {
-        handlerResult.catch(error =>
-          yargs.getUsageInstance().fail(null, error)
-        )
+        yargs.getUsageInstance().cacheHelpMessage()
+        handlerResult.catch(error => {
+          try {
+            yargs.getUsageInstance().fail(null, error)
+          } catch (err) {
+            // fail's throwing would cause an unhandled rejection.
+          }
+        })
       }
     }
 
diff --git a/lib/usage.js b/lib/usage.js
index fed6d9f..92bf378 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/lib/validation.js b/lib/validation.js
index 527a0d5..1a31e58 100644
--- a/lib/validation.js
+++ b/lib/validation.js
@@ -96,7 +96,7 @@ module.exports = function validation (yargs, usage, y18n) {
       if (specialKeys.indexOf(key) === -1 &&
         !positionalMap.hasOwnProperty(key) &&
         !yargs._getParseContext().hasOwnProperty(key) &&
-        !aliases.hasOwnProperty(key)
+        !self.isValidAndSomeAliasIsNotNew(key, aliases)
       ) {
         unknown.push(key)
       }
@@ -120,6 +120,21 @@ module.exports = function validation (yargs, usage, y18n) {
     }
   }
 
+  // check for a key that is not an alias, or for which every alias is new,
+  // implying that it was invented by the parser, e.g., during camelization
+  self.isValidAndSomeAliasIsNotNew = function isValidAndSomeAliasIsNotNew (key, aliases) {
+    if (!aliases.hasOwnProperty(key)) {
+      return false
+    }
+    const newAliases = yargs.parsed.newAliases
+    for (let a of [key, ...aliases[key]]) {
+      if (!newAliases.hasOwnProperty(a) || !newAliases[key]) {
+        return true
+      }
+    }
+    return false
+  }
+
   // validate arguments limited to enumerated choices
   self.limitedChoices = function limitedChoices (argv) {
     const options = yargs.getOptions()
diff --git a/yargs.js b/yargs.js
index 5b23a0f..794cf02 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


Test patch:
diff --git a/test/command.js b/test/command.js
index 9c394ce..d3a5628 100644
--- a/test/command.js
+++ b/test/command.js
@@ -1426,36 +1426,69 @@ 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 <pos>', '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 <pos>', '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) => {
+      const y = yargs('foo')
+        .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
+      // console output is suppressed. tldr; we capture
+      // the log output:
+      let errorLog = ''
+      y._getLoggerInstance().error = (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)
     })
   })
 
diff --git a/test/usage.js b/test/usage.js
index f3c9725..75918ac 100644
--- a/test/usage.js
+++ b/test/usage.js
@@ -810,6 +810,75 @@ describe('usage tests', () => {
       r.should.have.property('exit').and.equal(true)
     })
 
+    describe('with hyphens in options', () => {
+      it('fails when an invalid argument is provided', (done) => {
+        return yargs('--foo-bar')
+          .strict()
+          .fail((msg) => {
+            return done()
+          })
+          .argv
+      })
+
+      it('accepts valid options', () => {
+        const r = checkUsage(() => {
+          const opts = {
+            '--foo-bar': { description: 'foo bar option' },
+            '--bar-baz': { description: 'bar baz option' }
+          }
+
+          return yargs('--foo-bar --bar-baz')
+            .options(opts)
+            .strict()
+            .parse()
+        })
+
+        r.result.should.have.property('foo-bar', true)
+        r.result.should.have.property('fooBar', true)
+        r.result.should.have.property('bar-baz', true)
+        r.result.should.have.property('barBaz', true)
+      })
+
+      it('works with aliases', () => {
+        const r = checkUsage(() => {
+          const opts = {
+            '--foo-bar': { description: 'foo bar option', alias: 'f' },
+            '--bar-baz': { description: 'bar baz option', alias: 'b' }
+          }
+
+          return yargs('--foo-bar -b')
+            .options(opts)
+            .strict()
+            .parse()
+        })
+
+        r.result.should.have.property('foo-bar', true)
+        r.result.should.have.property('fooBar', true)
+        r.result.should.have.property('f', true)
+        r.result.should.have.property('bar-baz', true)
+        r.result.should.have.property('barBaz', true)
+        r.result.should.have.property('b', true)
+      })
+
+      it('accepts mixed options with values', () => {
+        const r = checkUsage(() => {
+          const opts = {
+            '--foo-bar': { description: 'foo bar option', demand: true },
+            '--baz': { description: 'baz option', demand: true }
+          }
+
+          return yargs('--foo-bar 150 --baz')
+            .options(opts)
+            .strict()
+            .parse()
+        })
+
+        r.result.should.have.property('foo-bar', 150)
+        r.result.should.have.property('fooBar', 150)
+        r.result.should.have.property('baz', true)
+      })
+    })
+
     it('should fail given an option argument without a corresponding description', () => {
       const r = checkUsage(() => {
         const opts = {