Accepting sync and async callbacks while keeping yargs functions sync-only is incoherent #1420

mleguen · 2019-09-04T08:47:29Z

This issue is a follow-up of the discusion with @bcoe and @gajus in #1412

Example with command handlers:

at the moment:
- we accept async handlers for commands
- we asynchronously log an error in parse() and showHelp() (and maybe other functions as well?) when an async handler reject
- as parse() and showHelp() are sync, the calling program has no way to know if and when such an error will occur
expected behavior:
- when an async handler needs to be called, the calling Yargs function (parse(), showHelp(), etc.) should return a promise, to be resolved/rejected after the handler completes, for the calling program to know what is happening
- when the handler is sync, the current behavior should be kept, ie the calling Yargs function should not return a promise

Before opening any PR, please detail in this issue the intended conception for review, and do not limit it to the command handler example detailed here (there may be other cases of async input).

The text was updated successfully, but these errors were encountered:

mleguen · 2019-09-11T10:37:43Z

Async middlewares would have to be taken into account as well in the conception.

mleguen · 2019-09-11T12:50:02Z

Where are promises currently used in Yargs?

command handler callbacks in command.runCommand():
- if the callback returns a promise, it is asynchronously checked by Yargs to emit a validation error if rejected
- the promise of this check is lost (neither stored nor returned by runCommand())
- calling applications never know if they are going to receive an asynchronous validation error, so strictly speaking, never know if the parsing is successful
- this behavior is not documented
middleware callbacks in middleware.applyMiddleware()
- if a middleware is to be applied before validation, asynchronous callback are forbidden
- if a callback returns a promise, the following middleware callbacks are ran asynchronously, once the promise is fulfilled
- the command handler is also ran asynchronously once the last middleware's promise is fulfilled
- the promise of the last middleware is returned by applyMiddleware(), runCommand(), etc., up to parse()
- this allows calling applications to know when middlewares are done, but is completely incoherent with the way async command handlers are handled
- calling applications still do not know if they are going to receive an asynchronous validation error afterwards, as the promise of the handler execution is still lost
- none of these behaviors are documented
completion functions in completion.getCompletion():
- if the completion function returns a promise, the done() callback is ran asynchronously
- the promise of done() execution is correctly returned by completion.getCompletion()
- however, it is lost in yargs.getCompletion() which does not return it
- it is also lost in yargs.pargs() which returns argv, although the done callback it passed to getCompletion() is eventualy going to exit the process

mleguen · 2019-09-18T12:17:17Z

@bcoe @gajus A beginning of proposal to discuss:

General approach

Wrapping maybe promise handling code

Several of our functions would need to work with "maybe promise" (name already used by our current "is-promise" module), ie inputs which are either promises, or not.

To avoid adding tests everywhere, and/or falling into a callback hell (we have no async in node 6, remember #1422), and to change as less code as possible, I suggest wrapping our code dealing with maybe promises with something like this:

function wrapMaybePromise(maybePromise, resolveHandler, rejectHandler) {
  return isPromise(maybePromise)
    ? maybePromise.then(resolveHandler, rejectHandler)
    : resolveHandler(maybePromise)
}

This would help us turn easily sync-only parts of our code, for example:

innerArgv = innerYargs._parseArgs(null, null, true, commandIndex)
aliases = innerYargs.parsed.aliases

// [...]

if (!yargs._hasOutput()) {
  positionalMap = populatePositionals(commandHandler, innerArgv, currentContext, yargs)
}

const middlewares = globalMiddleware.slice(0).concat(commandHandler.middlewares || [])
applyMiddleware(innerArgv, yargs, middlewares, true)

// [...]

return innerArgv

into maybe-promise-compatible code:

innerArgv = innerYargs._parseArgs(null, null, true, commandIndex)
return wrapMaybePromise(innerArgv, (innerArgv) => {
  aliases = innerYargs.parsed.aliases

  // [...]

  if (!yargs._hasOutput()) {
    positionalMap = populatePositionals(commandHandler, innerArgv, currentContext, yargs)
  }

  const middlewares = globalMiddleware.slice(0).concat(commandHandler.middlewares || [])
  applyMiddleware(innerArgv, yargs, middlewares, true)

  // [...]

  return innerArgv
})

This would also help us simplify some of our already boilerplate-code, such as:

innerArgv = applyMiddleware(innerArgv, yargs, middlewares, false)

let handlerResult
if (isPromise(innerArgv)) {
  handlerResult = innerArgv.then(argv => commandHandler.handler(argv))
} else {
  handlerResult = commandHandler.handler(innerArgv)
}

if (isPromise(handlerResult)) {
  // [...]
}

into:

innerArgv = applyMiddleware(innerArgv, yargs, middlewares, false)
handlerResult = wrapMaybePromise(innerArgv, argv => commandHandler.handler(argv))

if (isPromise(handlerResult)) {
  // [...]
}

Propagating maybe promises

Any function having maybe promises as input should output a maybe promise.

This would ensure:

that there would no longer be "normal" unhandled rejection warnings (either displayed, or masked by calling exit before it is displayed)
that calling applications could know when yargs is really done with the job it was given

while keeping an unchanged API for sync-only applications (except for showHelp(), which would non longer be chainable, but had no reason to IMHO).

To implement this, we should start from the identified maybe promise inputs (see "Where are promises currently used in Yargs?" above), and identify their propagation tree through functions, down to the API.

This will give us the list of all impacted functions to modify (including tests and documentation).

This is going to be my next work ;-)

mleguen · 2019-09-18T13:44:08Z

Here is the maybe promises propagation tree:

+-- commandHandler.handler()
|   +-- command.runCommand()
|       +-- yargs._parseArgs()
|           +-- command.runCommand() (recursion, see above)
|           +-- yargs.parse() (API)
|           |   +-- completion.getCompletion()
|           |       +-- yargs._parseArgs() (recursion, see above)
|           |       +-- yargs.getCompletion() (API)
|           +-- yargs.showHelp() (API)
|               +-- yargs._parseArgs() (recursion, see above)
|               +-- usage.fail()
|                   +-- command.runCommand() (recursion, see above)
|                   +-- command.postProcessPositionals()
|                   |   +-- command.populatePositionals()
|                   |       +-- command.runCommand() (recursion, see above)
|                   +-- validation.nonOptionCount()
|                   |   +-- yargs._runValidation()
|                   |       +-- command.runCommand() (recursion, see above)
|                   |       +-- yargs._parseArgs() (recursion, see above)
|                   +-- validation.positionalCount()
|                   |   +-- command.populatePositionals() (already listed above)
|                   +-- validation.requiredArguments()
|                   |   +-- yargs._runValidation() (already listed above)
|                   +-- validation.unknownArguments()
|                   |   +-- yargs._runValidation() (already listed above)
|                   +-- validation.limitedChoices()
|                   |   +-- yargs._runValidation() (already listed above)
|                   +-- validation.customChecks()
|                   |   +-- yargs._runValidation() (already listed above)
|                   +-- validation.implications()
|                   |   +-- yargs._runValidation() (already listed above)
|                   +-- validation.conflicting()
|                   |   +-- yargs._runValidation() (already listed above)
|                   +-- validation.recommendCommands()
|                       +-- yargs._parseArgs() (recursion, see above)
+-- middleware()
|   +-- middleware.applyMiddleware()
|       +-- command.runCommand() (already listed above)
+-- completionFunction()
    +-- completion.getCompletion() (already listed above)

Synthesis:

3 API functions would be impacted:
- yargs.getCompletion():
  - would return a maybe promise of void
  - meaning:
    - done called with the completion results
    - potential async warnings emitted
- yargs.parse():
  - would return a maybe promise of argv
  - meaning:
    - parsing was successful and returned argv
    - potential async warnings emitted
- yargs.showHelp():
  - would return a maybePromise of void
  - meaning:
    - help shown
    - potential async warnings emitted
17 internal functions would be impacted:
- command:
  - populatePositionals()
  - postProcessPositionals()
  - runCommand()
- completion.getCompletion()
- middleware.applyMiddleware()
- usage.fail()
- validation:
  - conflicting()
  - customChecks()
  - implications()
  - limitedChoices()
  - nonOptionCount()
  - positionalCount()
  - recommendCommands()
  - requiredArguments()
  - unknownArguments()
- yargs:
  - _parseArgs()
  - _runValidation()

Waiting for your feedback now before going ahead ;-)

bcoe · 2019-09-18T21:24:49Z

@mleguen I will make an effort to give feedback this weekend, appreciate the deep dive.

bcoe · 2019-09-18T21:29:26Z

at a glance, I think this is sounding like a well thought out proposal, could you provide some pseudo-code of what the API would look like, as an example I'm wondering if:

const a = yargs.command('command', 'description', () => {}, async () => {}).argv

Would mean that a is a promise rather than an object return? i.e., is the idea that the sync API will remain the same, but as soon as a promise is introduced anywhere, the resolution becomes async in general?

I like this idea, it would be a breaking change worth making.

mleguen · 2019-09-19T12:01:41Z

@bcoe you are right, I forgot to add an example of how to used the proposed API.

I like your idea of "as soon as a promise is introduced anywhere, the resolution becomes async in general", but however I see this is not what I proposed leads us to.

Take a little bit more complex example:

const b = yargs
  .command('one', 'description', () => {}, () => {})
  .command('two', 'description', () => {}, async () => {})
  .argv

b will be a "maybe promise", depending on passed arguments:

not a promise with "one" (sync handler)
a promise with "two" (async handler)

In your first example, depending on wether the command handler is called or not, a would be a promise or not.

cspotcode · 2019-09-21T01:05:00Z

Please consider adding Sync variants of the affected functions that will only return synchronous results, throwing an error if the result would be a promise.

These will better support static code analysis and editor tooling. They will be strictly opt-in; developers are free to use the existing methods and fields synchronously as outlined in this proposal. No one will be forced to use the sync variants.

const b = yargs./* command declared here */.argv;
// Editor infers that b is of type Promise<Argv> | Argv
const shouldForce = b.force; // <-- red quiggly line because we might need to await if it's a promise.

const b = yargs./* command declared here */.argvSync;
// Editor infers that b is of type Argv
const shouldForce = b.force; // works as expected; we know b cannot be a promise.

mleguen · 2019-09-25T07:55:21Z

@cspotcode Instead of sync variants of affected functions, would an option to prevent returning a promise do the job for you?

mleguen · 2019-09-25T09:16:01Z

This should answer @bcoe 's question about examples of the new API in use.

@cspotcode , I still have to add an example with the requested behavior to prevent returning a promise.

Future API usage examples

Sync-only

Sync-only applications would not be impacted:

try {
  const argv = yargs
    .command('syncCmdSuccess', 'description', () => {}, () => {})
    .command('syncCmdError', 'description', () => {}, () => {
      throw new Error()
    })
    .parse()
  // The syntax was incorrect or the selected command handler returned a value
  doWatheverYouWantWithParsingResults(argv)
} catch(err) {
  // The selected command handler threw
  dealWithHandlerErrors(err)
}

Async-only

Async-only applications would now be possible:

With async/await

Straightforward with async/await:

try {
  const argv = await yargs
    .command('asyncCmdSuccess', 'description', () => {}, () => Promise(
      (resolve) => setTimeout(() => resolve(), 500)
    ))
    .command('asyncCmdError', 'description', () => {}, () => Promise(
      (_, reject) => setTimeout(() => reject(), 500)
    ))
    .parse()
  // The syntax was incorrect or the selected command handler promise was resolved
  doWatheverYouWantWithParsingResults(argv)
} catch(err) {
  // The selected command handler promise was rejected
  dealWithHandlerErrors(err)
}

Without async/await

Not that difficult without async/await:

const argv = yargs
  .command('asyncCmdSuccess', 'description', () => {}, () => Promise(
    (resolve) => setTimeout(() => resolve(), 500)
  ))
  .command('asyncCmdError', 'description', () => {}, () => Promise(
    (_, reject) => setTimeout(() => reject(), 500)
  ))
  .parse()
  .then(
    // The syntax was incorrect or the selected command handler promise was resolved
    doWatheverYouWantWithParsingResults,
    // The selected command handler promise was rejected
    dealWithHandlerErrors
  )

Sync-async mix

With async/await

Straightforward as usual with async/await:

try {
  const argv = await yargs
    .command('asyncCmdSuccess', 'description', () => {}, () => Promise(
      (resolve) => setTimeout(() => resolve(), 500)
    ))
    .command('asyncCmdError', 'description', () => {}, () => Promise(
      (_, reject) => setTimeout(() => reject(), 500)
    ))
    .command('syncCmdSuccess', 'description', () => {}, () => {})
    .command('syncCmdError', 'description', () => {}, () => {
      throw new Error()
    })
    .parse()
  // The syntax was incorrect or the selected command handler returned a value or a promise which is now resolved
  doWatheverYouWantWithParsingResults(argv)
} catch(err) {
  // The selected command handler threw or returned a promise which is now rejected
  dealWithHandlerErrors(err)
}

Without async/await

A bit more complex without async/await, but still clean:

const argv = new Promise(resolve => resolve(
  yargs
    .command('asyncCmdSuccess', 'description', () => {}, () => Promise(
      (resolve) => setTimeout(() => resolve(), 500)
    ))
    .command('asyncCmdError', 'description', () => {}, () => Promise(
      (_, reject) => setTimeout(() => reject(), 500)
    ))
    .command('syncCmdSuccess', 'description', () => {}, () => {})
    .command('syncCmdError', 'description', () => {}, () => {
      throw new Error()
    })
    .parse()
))
.then(
  // The syntax was incorrect or the selected command handler returned a value or a promise which is now resolved
  doWatheverYouWantWithParsingResults,
  // The selected command handler threw or returned a promise which is now rejected
  dealWithHandlerErrors
)

The additional Promise layer here is used to turn the argv array, the promise of argv array or the exception resulting from parse() call into a promise of argv array, to handle its resolution or rejection in a single point.

mleguen · 2019-09-25T09:49:09Z

@cspotcode Here is what I propose for sync applications not wanting callbacks to return promises:

Future API usage examples (follow-up)

Sync-async mix with sync-only option

try {
  const argv = yargs
    .command('asyncCmdSuccess', 'description', () => {}, () => Promise(
      (resolve) => setTimeout(() => resolve(), 500)
    ))
    .command('asyncCmdError', 'description', () => {}, () => Promise(
      (_, reject) => setTimeout(() => reject(), 500)
    ))
    .command('syncCmdSuccess', 'description', () => {}, () => {})
    .command('syncCmdError', 'description', () => {}, () => {
      throw new Error()
    })
    .parseSync()
  // The syntax was incorrect or the selected command handler returned a value
  doWatheverYouWantWithParsingResults(argv)
} catch(err) {
  if (err instanceof YAsyncError) {
    // The selected command handler returned a promise
    dealWithHandlerReturningAPromiseWhenNotAllowed(err)
  } else {
    // The selected command handler threw
    dealWithHandlerErrors(err)
  }
}

EDIT: V2 taking @cspotcode 's concerns about IDE support into account.

cspotcode · 2019-09-25T15:31:05Z

The issue is supporting editor tooling and code completion. Using dynamic flags and toggles to change function signatures generally doesn't work, because the editor can't understand it without super-complicated type declarations. Using different function names is a simpler approach, because each function maintains a consistent signature.

…

On Wed, Sep 25, 2019, 5:49 AM Mael Le Guen ***@***.***> wrote: @cspotcode <https://github.com/cspotcode> Here is what I propose for sync applications not wanting callbacks to return promises: Future API usage examples (follow-up) Sync-async mix with sync-only option try { const argv = yargs .async(false) .command('asyncCmdSuccess', 'description', () => {}, () => Promise( (resolve) => setTimeout(() => resolve(), 500) )) .command('asyncCmdError', 'description', () => {}, () => Promise( (_, reject) => setTimeout(() => reject(), 500) )) .command('syncCmdSuccess', 'description', () => {}, () => {}) .command('syncCmdError', 'description', () => {}, () => { throw new Error() }) .parse() // The syntax was incorrect or the selected command handler returned a value doWatheverYouWantWithParsingResults(argv) } catch(err) { if (err instanceof YAsyncError) { // The selected command handler returned a promise dealWithHandlerReturningAPromiseWhenNotAllowed(err) } else { // The selected command handler threw dealWithHandlerErrors(err) } } — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1420?email_source=notifications&email_token=AAC35OCD2L6UXG2EQGSWSCDQLMXZVA5CNFSM4ITPDSE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7RJUOY#issuecomment-534944315>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC35OFYPXJSVQTPLXZYJ63QLMXZVANCNFSM4ITPDSEQ> .

mleguen · 2019-10-02T16:02:43Z

@cspotcode I edited my example above to add a parseSync function as requested (helpSync and getCompletionSync would also be added too).

If there are no other suggestions/objections, I will start working on this next week.

cspotcode · 2019-10-02T20:18:21Z

@mleguen looks good to me, thanks!

mleguen · 2019-10-09T13:21:01Z

To stay coherent with @cspotcode 's request to add a parseSync function, I am also adding a parseAsync function, which will always return a promise (easier to handle for users not yet using async/await).

The general recommendations would become: use parseAsync in most cases, use parseSync if you absolutely need to stay sync. And we could imagine deprecating parse in a future version.

mleguen · 2019-10-09T17:48:52Z

As node 6 support is going to be dropped in the next major version (#1438 ), we no longer need all the stuff about "maybePromises" described above. I will use async/await instead.

bcoe · 2021-04-04T04:13:26Z

The work in #1876 should address the issues raised in this thread.

It can currently be installed via npm i yargs@next.

bcoe added the discussion label Sep 5, 2019

shadowspawn mentioned this issue Oct 3, 2019

Async typescript functions as actions tj/commander.js#806

Closed

mleguen mentioned this issue Oct 9, 2019

WIP: feature(parse): sync/async API #1443

Closed

mleguen mentioned this issue Oct 9, 2019

Deprecate Node.js 6.x? #1438

Closed

mleguen mentioned this issue Nov 20, 2019

Async API in the same module as the sync one #1487

Closed

3 tasks

bcoe mentioned this issue Nov 25, 2019

docs: describe yargs.async API surface #1491

Closed

bcoe mentioned this issue Dec 6, 2020

feat!: improve support for async/await #1823

Merged

10 tasks

bcoe closed this as completed Apr 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accepting sync and async callbacks while keeping yargs functions sync-only is incoherent #1420

Accepting sync and async callbacks while keeping yargs functions sync-only is incoherent #1420

mleguen commented Sep 4, 2019

mleguen commented Sep 11, 2019

mleguen commented Sep 11, 2019

mleguen commented Sep 18, 2019

mleguen commented Sep 18, 2019

bcoe commented Sep 18, 2019

bcoe commented Sep 18, 2019

mleguen commented Sep 19, 2019

cspotcode commented Sep 21, 2019

mleguen commented Sep 25, 2019

mleguen commented Sep 25, 2019

mleguen commented Sep 25, 2019 •

edited

cspotcode commented Sep 25, 2019 via email

mleguen commented Oct 2, 2019

cspotcode commented Oct 2, 2019

mleguen commented Oct 9, 2019

mleguen commented Oct 9, 2019

bcoe commented Apr 4, 2021

Accepting sync and async callbacks while keeping yargs functions sync-only is incoherent #1420

Accepting sync and async callbacks while keeping yargs functions sync-only is incoherent #1420

Comments

mleguen commented Sep 4, 2019

mleguen commented Sep 11, 2019

mleguen commented Sep 11, 2019

mleguen commented Sep 18, 2019

General approach

Wrapping maybe promise handling code

Propagating maybe promises

mleguen commented Sep 18, 2019

bcoe commented Sep 18, 2019

bcoe commented Sep 18, 2019

mleguen commented Sep 19, 2019

cspotcode commented Sep 21, 2019

mleguen commented Sep 25, 2019

mleguen commented Sep 25, 2019

Future API usage examples

Sync-only

Async-only

With async/await

Without async/await

Sync-async mix

With async/await

Without async/await

mleguen commented Sep 25, 2019 • edited

Future API usage examples (follow-up)

Sync-async mix with sync-only option

cspotcode commented Sep 25, 2019 via email

mleguen commented Oct 2, 2019

cspotcode commented Oct 2, 2019

mleguen commented Oct 9, 2019

mleguen commented Oct 9, 2019

bcoe commented Apr 4, 2021

mleguen commented Sep 25, 2019 •

edited