From 65cb6a72abed0b0d09257ff5ec4a2c899f900104 Mon Sep 17 00:00:00 2001 From: Landon Yarrington Date: Sun, 10 Apr 2022 15:19:18 -0600 Subject: [PATCH 1/3] fix: handle result of async middleware --- lib/command.ts | 86 +++++++++++++++++++++++++++++++++------------ test/middleware.cjs | 28 +++++++++++++++ 2 files changed, 91 insertions(+), 23 deletions(-) diff --git a/lib/command.ts b/lib/command.ts index b588b7e40..4345f9114 100644 --- a/lib/command.ts +++ b/lib/command.ts @@ -364,34 +364,16 @@ export class CommandInstance { pc.push(c); return `$0 ${pc.join(' ')}`; } - private applyMiddlewareAndGetResult( + private handleValidationAndGetResult( isDefaultCommand: boolean, commandHandler: CommandHandler, innerArgv: Arguments | Promise, currentContext: Context, - helpOnly: boolean, aliases: Dictionary, - yargs: YargsInstance - ): Arguments | Promise { - let positionalMap: Dictionary = {}; - // If showHelp() or getHelp() is being run, we should not - // execute middleware or handlers (these may perform expensive operations - // like creating a DB connection). - if (helpOnly) return innerArgv; - if (!yargs.getInternalMethods().getHasOutput()) { - positionalMap = this.populatePositionals( - commandHandler, - innerArgv as Arguments, - currentContext, - yargs - ); - } - const middlewares = this.globalMiddleware - .getMiddleware() - .slice(0) - .concat(commandHandler.middlewares); - innerArgv = applyMiddleware(innerArgv, yargs, middlewares, true); - + yargs: YargsInstance, + middlewares: Middleware[], + positionalMap: Dictionary + ) { // we apply validation post-hoc, so that custom // checks get passed populated positional arguments. if (!yargs.getInternalMethods().getHasOutput()) { @@ -453,6 +435,64 @@ export class CommandInstance { return innerArgv; } + private applyMiddlewareAndGetResult( + isDefaultCommand: boolean, + commandHandler: CommandHandler, + innerArgv: Arguments, + currentContext: Context, + helpOnly: boolean, + aliases: Dictionary, + yargs: YargsInstance + ): Arguments | Promise { + let positionalMap: Dictionary = {}; + // If showHelp() or getHelp() is being run, we should not + // execute middleware or handlers (these may perform expensive operations + // like creating a DB connection). + if (helpOnly) return innerArgv; + if (!yargs.getInternalMethods().getHasOutput()) { + positionalMap = this.populatePositionals( + commandHandler, + innerArgv as Arguments, + currentContext, + yargs + ); + } + const middlewares = this.globalMiddleware + .getMiddleware() + .slice(0) + .concat(commandHandler.middlewares); + + const maybePromiseArgv = applyMiddleware( + innerArgv, + yargs, + middlewares, + true + ); + + return isPromise(maybePromiseArgv) + ? maybePromiseArgv.then(resolvedInnerArgv => + this.handleValidationAndGetResult( + isDefaultCommand, + commandHandler, + resolvedInnerArgv, + currentContext, + aliases, + yargs, + middlewares, + positionalMap + ) + ) + : this.handleValidationAndGetResult( + isDefaultCommand, + commandHandler, + maybePromiseArgv, + currentContext, + aliases, + yargs, + middlewares, + positionalMap + ); + } // transcribe all positional arguments "command [apple]" // onto argv. private populatePositionals( diff --git a/test/middleware.cjs b/test/middleware.cjs index 7e0f5439f..179e4612d 100644 --- a/test/middleware.cjs +++ b/test/middleware.cjs @@ -281,6 +281,34 @@ describe('middleware', () => { } }); + // Addresses: https://github.com/yargs/yargs/issues/2124 + // This test will fail if the result of async middleware is not treated like a promise + // eslint-disable-next-line no-restricted-properties + it.only('treats result of async middleware as promise', done => { + const input = 'cmd1 -f Hello -b world'; + yargs(input) + .command( + 'cmd1', + 'cmd1 desc', + yargs => + yargs + .option('foo', {type: 'string', alias: 'f', required: true}) + .option('bar', {type: 'string', alias: 'b', required: true}) + .middleware(async argv => { + await new Promise(r => setTimeout(r, 5)); + return argv; + }, true), + _argv => { + done(); + } + ) + .fail(msg => { + done(new Error(msg)); + }) + .strict() + .parse(); + }); + it('runs before validation, when middleware is added in builder', done => { yargs(['mw']) .command( From 6a99fb1f5ace771d22fce69b97283592384f57b0 Mon Sep 17 00:00:00 2001 From: Landon Yarrington Date: Sun, 10 Apr 2022 15:23:06 -0600 Subject: [PATCH 2/3] cleanup --- test/middleware.cjs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/middleware.cjs b/test/middleware.cjs index 179e4612d..5e82e64e9 100644 --- a/test/middleware.cjs +++ b/test/middleware.cjs @@ -283,8 +283,7 @@ describe('middleware', () => { // Addresses: https://github.com/yargs/yargs/issues/2124 // This test will fail if the result of async middleware is not treated like a promise - // eslint-disable-next-line no-restricted-properties - it.only('treats result of async middleware as promise', done => { + it('treats result of async middleware as promise', done => { const input = 'cmd1 -f Hello -b world'; yargs(input) .command( From fca98f43792a533e96a0c0cbd248df6d86757ce8 Mon Sep 17 00:00:00 2001 From: Landon Yarrington Date: Thu, 21 Apr 2022 11:38:07 -0700 Subject: [PATCH 3/3] Simplify test --- test/middleware.cjs | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/test/middleware.cjs b/test/middleware.cjs index 5e82e64e9..37fe9ffad 100644 --- a/test/middleware.cjs +++ b/test/middleware.cjs @@ -282,21 +282,14 @@ describe('middleware', () => { }); // Addresses: https://github.com/yargs/yargs/issues/2124 - // This test will fail if the result of async middleware is not treated like a promise - it('treats result of async middleware as promise', done => { - const input = 'cmd1 -f Hello -b world'; + // This test will fail if result of async middleware is not handled like a promise + it('does not cause an unexpected error when async middleware and strict are both used', done => { + const input = 'cmd1'; yargs(input) .command( 'cmd1', 'cmd1 desc', - yargs => - yargs - .option('foo', {type: 'string', alias: 'f', required: true}) - .option('bar', {type: 'string', alias: 'b', required: true}) - .middleware(async argv => { - await new Promise(r => setTimeout(r, 5)); - return argv; - }, true), + yargs => yargs.middleware(async argv => argv, true), _argv => { done(); }