Async API in the same module as the sync one

[mleguen]

As discussed in #1478 , the async API of yargs should be accessible as follows:

const yargsa = require('yargs').yargsa

which would increase code reuse between the sync and async API, and the project maintenability, compared to the first approaches described in #1420 and yargs/yargsa#4.

Todo:

• describe the future API (ussage examples)
• describe the intended approach (dev and tests)
• do it

[mleguen]

Future API usage examples

With .exitProcess(true) (default)

const yargsa = require('yargs').yargsa

async main() {
  const argv = await yargsa
    .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()
  // Parsing is OK and the handler returned (if sync) or its promise was resolved (if async)
  doWatheverYouWantWithParsingResults(argv)
  // No try/catch, as the process would be exited before the catch
  // and before node could emit an "unhandled rejection" warning
}

main()

With .exitProcess(false)

const yargsa = require('yargs').yargsa

async main() {
  try {
    const argv = await yargsa
      .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()
      })
      .exitProcess(false)
      .parse()
    // Parsing is OK and the handler returned (if sync) or its promise was resolved (if async)
    doWatheverYouWantWithParsingResults(argv)
  }
  // Without try/catch, node would emit an "unhandled rejection" warning if parsing is KO
  catch(err) {
    // The syntax is incorrect or the handler threw (if sync) or its promise was rejected (if async)
    dealWithParsingErrors(err)
  }  
}

main()

[bcoe]

@mleguen are you thinking this replaces yargsa as its own module, or should we still publish it?

[mleguen]

@bcoe I do no longer see at the moment any use for a separate yargsa module.

[bcoe]

@mleguen @petrgrishin I've been thinking a bit today about the async behavior in yargs and yargsa (which I've come around to thinking was a bad idea.).

I agree with @mleguen's original point in #1420, that the library's mixing of sync and async paradigms creates for a confusing experience...

I landed promise support in a few places when users submitted patches for the functionality, without thinking too deeply about the inherit complexity it introduced ... unfortunately this complexity is introduced for a feature I suspect is used by a small percentage of yargs' users.

Part of why I suggested the yargsa approach, was that I thought we could support these folks without overhauling the library significantly -- yargs has literally millions of users, most of whom are fine with the sync API, I don't love the idea of disrupting them in the name of an async API surface.

Where I Find Myself Landing

A synchronous API is fine for argument parsing (which usually takes ms, and happens early in the application's lifecycle) ... we have millions of folks using this library, and don't have an army of people banging on the door asking for the annoyance of calling async/await.

However... I think @mleguen's suggestions point out the potential for a major improvement to the library:

1. let's drop async functionality from the sync version of this library (no async middleware, no async handlers).

   I'm on the fence about async completion logic, since this is a special flow through the application, perhaps we carve out an exception.

2. If you want async command handlers, async builders, async middleware, you do this:

   const yargs = require('yargs').async

I think this is getting close to @mleguen's proposal in this issue, and his original suggestions in #1420... The main difference in my thinking, is that I'm not proposing this would ever replace the synchronous version of yargs.

Rather, it's additional behavior you opt in to (with additional constraints imposed that make the behavior more logical).

Next Steps

Rather than moving directly to landing code, as proposed in #1488, I suggest we work on a PR that fleshes out the API in docs/async.md, I've created a PR kicking off this process here.

[mleguen]

@bcoe @coreyfarrell If:

• we want an async API
• we want to have as less code and tests duplication as possible between the sync and async API
• we do not intend to deprecate the sync API
• and we want to impact as less as possible sync code and tests

then I think that even the require('yargs').async approach is not the good one. It would indeed require:

• either to duplicate code and tests (which would not be that problematic if we intended to deprecate the sync API)
• or to refactor heavily sync code and tests (which would not be that problematic if we did not want to impact as less as possible sync code and tests)

With all these constraints, why not simply adding xxxAsync method to the current API (parseAsync, etc.)? We would then limit duplication or refactoring to these methods, not to the whole API.

[bcoe]

With all these constraints, why not simply adding xxxAsync method to the current API (parseAsync, etc.)? We would then limit duplication or refactoring to these methods, not to the whole API.

Where I feel it makes sense for yargs to have async functionality is in little niceties like this:

const argv = yargs.command('fetch <url>', 'fetch the contents of a URL', () => {}, async (argv) => {
  const res = await fetch(argv.url)
  console.info(`status = ${res.status} ${res.statusText}`)
  console.info(await res.text())
}).argv

☝️ It's nice to be able to provide an async handler, and have it just work.

I don't think it would be as elegant to have to call yargs.asyncCommand, and we'd now be in a position where the command directory feature had to have a similar way to indicate that it's an async version ... I can imagine this getting ugly.

There's a precedent in the Node.js community of having a sync/async version of the module:

• mkdirp: defaults to an async API, but provides mkdirp.sync(dir, opts) for synchronous operations.
• rimraf: does the same.
• glob.

☝️ this pattern of having a separate sync vs, async export of the library is idiomatic to the community and I think would suit us well.

What I'm getting at

If someone runs:

const yargs = require('yargs')

They get the same API surface they have today, except you can no longer pass in promises to handlers, middleware, etc.

If someone runs:

const yargs = require('yargs').async

You can pass in async middleware, builders, and handlers, but the library resolves a promise on parse and argv rather than an object. This promise doesn't resolve until the stack of promises completes.

I think this distinction could put us in a position to offer an API that supports both worlds really well.

[mleguen]

Closed in favor of the discussion in #1491.