From a93f5ff35d7c09b01e0ca93d7d855d2b26593165 Mon Sep 17 00:00:00 2001 From: "Benjamin E. Coe" Date: Fri, 2 Apr 2021 17:46:27 -0700 Subject: [PATCH] fix(async): don't call parse callback until async ops complete (#1896) If a callback is provided to parse() it will now be executed after all promises have resolved. The cause of #1888, was that this[kUnfreeze](); was being called too soon, which set this.#parseFn to null. Fixes #1888 --- lib/yargs-factory.ts | 27 +++++++++++++++++++--- test/command.cjs | 14 +++++++++--- test/middleware.cjs | 2 +- test/yargs.cjs | 53 ++++++++++++++------------------------------ 4 files changed, 53 insertions(+), 43 deletions(-) diff --git a/lib/yargs-factory.ts b/lib/yargs-factory.ts index d83767608..54c1c3ba8 100644 --- a/lib/yargs-factory.ts +++ b/lib/yargs-factory.ts @@ -1057,8 +1057,29 @@ export class YargsInstance { !!shortCircuit ); this.#completion!.setParsed(this.parsed as DetailedArguments); - if (this.#parseFn) this.#parseFn(this.#exitError, parsed, this.#output); - this[kUnfreeze](); // Pop the stack. + if (isPromise(parsed)) { + return parsed + .then(argv => { + if (this.#parseFn) this.#parseFn(this.#exitError, argv, this.#output); + return argv; + }) + .catch(err => { + if (this.#parseFn) { + this.#parseFn!( + err, + (this.parsed as DetailedArguments).argv, + this.#output + ); + } + throw err; + }) + .finally(() => { + this[kUnfreeze](); // Pop the stack. + }); + } else { + if (this.#parseFn) this.#parseFn(this.#exitError, parsed, this.#output); + this[kUnfreeze](); // Pop the stack. + } return parsed; } parserConfiguration(config: Configuration) { @@ -2262,7 +2283,7 @@ interface FrozenYargsInstance { interface ParseCallback { ( err: YError | string | undefined | null, - argv: Arguments | Promise, + argv: Arguments, output: string ): void; } diff --git a/test/command.cjs b/test/command.cjs index 32787b42c..f514d4760 100644 --- a/test/command.cjs +++ b/test/command.cjs @@ -2054,9 +2054,17 @@ describe('Command', () => { assert.strictEqual(set, true); }); }); - // TODO: investigate why .parse('cmd --help', () => {}); does not - // work properly with an async builder. We should test the same - // with handler. + // Refs: https://github.com/yargs/yargs/issues/1894 + it('does not print to stdout when parse callback is provided', async () => { + await yargs() + .command('cmd ', 'a test command', async () => { + await wait(); + }) + .parse('cmd --help', (_err, argv, output) => { + output.should.include('a test command'); + argv.help.should.equal(true); + }); + }); }); describe('builder', () => { diff --git a/test/middleware.cjs b/test/middleware.cjs index c607cfcec..04664b701 100644 --- a/test/middleware.cjs +++ b/test/middleware.cjs @@ -857,7 +857,7 @@ describe('middleware', () => { choices: [10, 20, 30], }) .coerce('foo', async arg => { - wait(); + await wait(); return (arg *= 2); }) .parse('--foo 2'), diff --git a/test/yargs.cjs b/test/yargs.cjs index bb3806a94..1840fda46 100644 --- a/test/yargs.cjs +++ b/test/yargs.cjs @@ -2687,7 +2687,7 @@ describe('yargs dsl tests', () => { // See: https://github.com/yargs/yargs/issues/1420 describe('async', () => { describe('parse', () => { - it('returns promise that resolves argv on success', done => { + it('calls parse callback once async handler has resolved', done => { let executionCount = 0; yargs() .command( @@ -2705,15 +2705,13 @@ describe('yargs dsl tests', () => { } ) .parse('cmd foo', async (_err, argv) => { - (typeof argv.then).should.equal('function'); - argv = await argv; argv.addedAsync.should.equal(99); argv.str.should.equal('foo'); executionCount.should.equal(1); return done(); }); }); - it('returns deeply nested promise that resolves argv on success', done => { + it('calls parse callback once deeply nested promise has resolved', done => { let executionCount = 0; yargs() .command( @@ -2738,15 +2736,13 @@ describe('yargs dsl tests', () => { () => {} ) .parse('cmd foo orange', async (_err, argv) => { - (typeof argv.then).should.equal('function'); - argv = await argv; argv.addedAsync.should.equal(99); argv.apple.should.equal('orange'); executionCount.should.equal(1); return done(); }); }); - it('returns promise that can be caught if rejected', done => { + it('populates err with async rejection', done => { let executionCount = 0; yargs() .command( @@ -2762,15 +2758,10 @@ describe('yargs dsl tests', () => { }); } ) - .parse('cmd foo', async (_err, argv) => { - (typeof argv.then).should.equal('function'); - try { - await argv; - } catch (err) { - err.message.should.equal('async error'); - executionCount.should.equal(1); - return done(); - } + .parse('cmd foo', async err => { + err.message.should.equal('async error'); + executionCount.should.equal(1); + return done(); }); }); it('caches nested help output, so that it can be output by showHelp()', done => { @@ -2789,16 +2780,11 @@ describe('yargs dsl tests', () => { }); } ).parse('cmd foo', async (_err, argv) => { - (typeof argv.then).should.equal('function'); - try { - await argv; - } catch (err) { - y.showHelp(output => { - output.should.match(/a command/); - executionCount.should.equal(1); - return done(); - }); - } + y.showHelp(output => { + output.should.match(/a command/); + executionCount.should.equal(1); + return done(); + }); }); }); it('caches deeply nested help output, so that it can be output by showHelp()', done => { @@ -2824,16 +2810,11 @@ describe('yargs dsl tests', () => { }, () => {} ).parse('cmd inner bar', async (_err, argv) => { - (typeof argv.then).should.equal('function'); - try { - await argv; - } catch (err) { - y.showHelp(output => { - output.should.match(/inner command/); - executionCount.should.equal(1); - return done(); - }); - } + y.showHelp(output => { + output.should.match(/inner command/); + executionCount.should.equal(1); + return done(); + }); }); }); });