Async command handlers

[zenflow]

Addresses issue #510 "Allow command handler to accept a promise" "Support async (i.e. promise-returning) command handlers"

we'd just need to update the argument validation and invocation of a command handler to understand a promise

This PR handles "update [...] invocation of a command handler to understand a promise". Nothing is done if the promise resolves, but if it rejects, it is handled as "failure" with an err but no msg.

@bcoe Not sure what needs to be done about "update the argument validation"... It seems like there's no change needed; arguments to command handlers, and validation of said arguments, aren't affected by this feature. This feature deals only with what is returned by the command handler (a promise, potentially). Am I missing something?

This PR also makes a slight change to the default failure handler, so that if there's no msg but there is an err, err will be logged instead, which will display the full stack trace for debugging.

Thank you for your work on this project, and taking the time to review this change :)

[bcoe]

this is looking good 👍

[bcoe]

I think it might be worth adding a test for the happy path too (the promise resolving).

[zenflow]

Sure, but I can't think of anything to assert in that case... This feature does nothing when the handler result is thenable and it resolves.

I had actually fixed (?) my resolution handler in lib/command.js from a () => {} noop to null so that it would not show as an untested code path in the test coverage reports.

Anyone have a suggestion of what to assert? What do we need to watch out for in this case?

[bcoe]

hey, made comment above with recommendation about assertion; I suppose now that the fail handler is called async, you can just call done() here after your promise resolves?

[bcoe]

I would probably just fail with err.message rather than null, I think your changes in usage.js probably wouldn't be needed then?

[zenflow]

I'm passing null here and making the changes in usage.js so that the error with stack trace gets logged to the console, similar to what happens when an error is thrown in a sync command handler.

(On that note, maybe we should align how errors are handled in sync vs async command handlers, and handle sync errors as "failures" too?)

It seems important to have the stack trace logged to the console, and it seems like this is the simplest way to achieve it.

I should probably add a test to make sure this happens. I will add one if you agree that that's what we want to happen.

[bcoe]

sorry for the slow reply -- this logic seems reasonable to me. Let's add a test for both this and the happy path. To test the happy path, I would just have a command handler that modifies a variable, and assert that it executed as expected:

let asyncHandlerCalled = false
const asyncHandler = new Promise((resolve, reject) => {
  asyncHandlerCalled = true
  return resolve()
})
yargs('foo')
  .command('foo', 'foo command', noop, (yargs) => Promise.reject(error))

something like that; you'll need to make sure the test is async, etc.

[zcei]

Looks really promising! 👏 (pun intended 😉 )

Not sure what needs to be done about "update the argument validation"... It seems like there's no change needed

This is a whole different topic, as async argument validation would turn whole yargs library into getting async, as the .argv cannot be available before the async validation succeeded.

As a workaround, you could throw a specific error (e.g. UnmatchedAsyncConstraint) and handle this in the .fail method (i.e. show the usage help text), together with your generic failure handling.

So getting this enhancement in without async argument validation would be awesome!

[zenflow]

@zcei It's promising, and also thenabling 😉 (Jokes, How long have you been a dad btw? My kids are 8 and 10, Jokes)

This is a whole different topic, as async argument validation would turn whole yargs library into getting async, as the .argv cannot be available before the async validation succeeded.

Ah, that explains it. Thanks. (D'uh)

Yeah changing yargs to do validation asynchronously seems like it have to have some significant breaking changes, since (correct me if I'm getting this wrong) it must be either always async or always sync, or Zalgo get's released.

Maybe we should open a separate issue for that?

[bcoe]

updated with comments, sorry for the slow turn around.

[bcoe]

sorry for the slow reply -- this logic seems reasonable to me. Let's add a test for both this and the happy path. To test the happy path, I would just have a command handler that modifies a variable, and assert that it executed as expected:

let asyncHandlerCalled = false
const asyncHandler = new Promise((resolve, reject) => {
  asyncHandlerCalled = true
  return resolve()
})
yargs('foo')
  .command('foo', 'foo command', noop, (yargs) => Promise.reject(error))

something like that; you'll need to make sure the test is async, etc.

[bcoe]

hey, made comment above with recommendation about assertion; I suppose now that the fail handler is called async, you can just call done() here after your promise resolves?

[bcoe]

@zenflow think you'll have a chance to see this over the finish line? otherwise hopefully I'll have some time over the holiday.

[zenflow]

@bcoe Sorry for all the delay - I'm finally free from work for a while now, after a few mad weeks of finishing up, documenting, handing off, etc.

I'll definitely have time to finish this off over the next week couple weeks :)

[bcoe]

@zenflow @zcei please try:

npm i yargs@next, which has the async command handler support; if everything is looking good in this release I will promote things soon.

[evocateur]

One (un-documented) side-effect of this change is that .parse() no longer stops the internal logger from dumping the error (self._hasParseCallback()) or implicitly set .exitProcess(false). The former smells like a bug to me, the latter probably wasn't the best choice, and in any case, the only way since v10.1.0 to catch a handler rejection or a yargs validation error (or string 😭) from the same method (when using .parse()) is to chain a fail() callback before calling .parse().

TL;DR: If you use async command handlers with .parse(), you must use a chained .fail() callback to ensure you catch all errors thrown during yargs execution.

[kyeotic]

@evocateur What does a "chained fail callback look like"? Is that chained onto parse or the callback?

[evocateur]

The yargs API method ".fail()". The parse() method only accepts a callback, it does not return a promise.

…

On Aug 1, 2018, at 21:01, Tim Kye ***@***.***> wrote: @evocateur What does a "chained fail callback look like"? Is that chained onto parse or the callback? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[remorses]

the typescript types should also be updated

[shaoner]

the typescript types should also be updated

yes i've just run into the same issue

[tianyingchun]

what' progress of this issue?

[mleguen]

@tianyingchun About updating types?

[tianyingchun]

@mleguen oh sorry, paste question to wrong place
my qeustion is ``yargs.parse(argv,callback) issues call back before handler promise is finished. #1069
#1069