Add parseAsync

[shadowspawn]

Pull Request

Problem

People are using async routines from TypeScript and Javascript, and want to declare async action handlers. For the caller to properly wind up the async calls, Commander needs to expose something to allow handling the asynchronous calls.

See #806

Solution

Add a parseAsync() call. It calls the normal parse, but then returns a Promise which includes the possibly async action handler. The client can then deal with the Promise in the normal way.

It is ok to have a mixture of async and non-async action handlers. Calling parseAsync works with either. Basically, if you have any async action handlers then you should call parseAsync instead of parse.

With this TypeScript code:

import * as commander from 'commander';

function sleep(ms: number) {
  return new Promise(resolve => setTimeout(resolve, ms));
}

async function run() {
  console.log('run+');
  await sleep(2000);
  console.log('run-');
}

const program = new commander.Command();
program
  .command("wait")
  .action(run);


program.parseAsync(['node', 'foo', 'wait']).then(() => {
  console.log('parseAsync success');
}).catch(err => {
  console.log('parseAsync failed');
});

console.log('Reached end of program');

See this:

$ npx tsc async.ts && node async.js
run+
Reached end of program
run-
parseAsync success

[shadowspawn]

To do:

• add tests
• investigate compatibility with node 6
• do some testing from JavaScript (initial development used TypeScript)
• README

Note: I am not a Promise expert. If there are bad patterns or assumptions in this code, then please let me know!

[jdmarshall]

I think I agree that the problem traces to the end of the event listener in action(), but if I back up for a second I have two other concerns.

The simplest one is, wouldn't it be simpler to register a new event that is emitted after fn.apply() resolves/returns.
The harder question is why action.apply() in an event handler in the first place? That makes propagating errors (and actions will have errors, especially when you are writing them!) much more problematic.

I know I wrote an async action. If you give me an event to wait for, I can promisify it in a couple lines of code and only call process.exit() after it resolves, and I think I'd be perfectly okay with that. In fact if I have multiple commands, which I do, that will still result in simpler code than my weird setInterval workaround.

[shadowspawn]

I think we have two different problems @jdmarshall.

I was focused on caller being able to manage async action handlers, and (say) await actions being completing before proceeding with other code. My intention was to make it possible and the rest was up to the caller.

Your problem (with setInterval workaround) is presumably that a pending promise alone does not keep node running, which is potentially unexpected:

• Nodejs does not wait for promise resolution - exits instead nodejs/node#22088

I didn't encounter the early-exit problem because I was using setTimeout as my async example. I am not sure if it is up to Commander to keep node running forever, but it might be reasonable to keep it running for some default period (like say 10s, which is the default timeout on docker stop).

[shadowspawn]

@jdmarshall Do you have a small example of a problem async action that would not run correctly for you if you could use await, without additional timer? (i.e. what is the code doing asynchronously that node does not recognise will resolve later?)

[jdmarshall]

Not at present. What drew me here was that I had an async action that seemed to be working fine, performing a lot of async calls (throttled uploads), and then I added a second async task to the end of that action and it never ran.

I haven't been back to it yet to come up with a repro case.

[shadowspawn]

Here is a javascript example for node 8 or up with await. Lots of logging!

const program = require('commander');

function sleep(ms) {
  return new Promise(resolve => setTimeout(resolve, ms));
}

async function doSomethingAsync() {
  console.log('doSomethingAsync+');
  await sleep(1000);
  console.log('doSomethingAsync-');
}

async function main() {
  console.log('main+');
  program
    .command('example')
    .action(doSomethingAsync);
  console.log('Calling parseAsync');
  const promise = program.parseAsync(['node', 'script', 'example']);
  console.log('Working before waiting');
  await promise;
  console.log('main-');
}

console.log('start program');
main().then(() => {
  console.log('main succeeded');
}).catch(() => {
  console.log('main failed');
})
console.log('finish program');

$ node example.js
start program
main+
Calling parseAsync
doSomethingAsync+
Working before waiting
finish program
doSomethingAsync-
main-
main succeeded

[blendsdk]

I have tested this PR and it works for me. I am on node v.10.14.2.
I am running async actions against a lengthy database update script.

[shadowspawn]

Thanks @blendsdk

[shadowspawn]

Added README. Have a test and typings.
Moving out of draft.

This is a non-breaking addition so can go in v4, but I am happy to leave it for v5 if preferred.

[shadowspawn]

This is the only PR I am interested in including before v4.1, but happy to leave it for v5 whether for safety or to allow more time to review @abetomo . No worries.

[abetomo]

LGTM!

[shadowspawn]

Thanks @abetomo. I'll start preparing 4.1 soon.

[shadowspawn]

CHANGELOG updated for 4.1 on develop branch. Ready to release to master, version number and tag still to be updated.

npm version minor

I'll add an issue for last change to Chinese README but we can add that after release.