From 395bb67749787d269cabe80ffc3133c2f6958aeb Mon Sep 17 00:00:00 2001 From: "Benjamin E. Coe" Date: Sun, 11 Apr 2021 19:46:24 -0700 Subject: [PATCH] fix(builder): apply default builder for showHelp/getHelp (#1913) The builder for default commands was not being properly applied when .showHelp() and .getHelp() were used. Fixes #1912 --- lib/command.ts | 34 ++++++++------- lib/yargs-factory.ts | 22 ++++++---- package.json | 2 +- test/usage.cjs | 99 ++++++++++++++++++++++++++++++++++++++++++++ test/yargs.cjs | 1 - 5 files changed, 135 insertions(+), 23 deletions(-) diff --git a/lib/command.ts b/lib/command.ts index 727d32a44..83bc17fa5 100644 --- a/lib/command.ts +++ b/lib/command.ts @@ -209,12 +209,13 @@ export class CommandInstance { this.defaultCommand; const currentContext = yargs.getInternalMethods().getContext(); const parentCommands = currentContext.commands.slice(); + const isDefaultCommand = !command; if (command) { currentContext.commands.push(command); currentContext.fullCommands.push(commandHandler.original); } const builderResult = this.applyBuilderUpdateUsageAndParse( - command, + isDefaultCommand, commandHandler, yargs, parsed.aliases, @@ -226,7 +227,7 @@ export class CommandInstance { if (isPromise(builderResult)) { return builderResult.then(result => { return this.applyMiddlewareAndGetResult( - command, + isDefaultCommand, commandHandler, result.innerArgv, currentContext, @@ -237,7 +238,7 @@ export class CommandInstance { }); } else { return this.applyMiddlewareAndGetResult( - command, + isDefaultCommand, commandHandler, builderResult.innerArgv, currentContext, @@ -248,7 +249,7 @@ export class CommandInstance { } } private applyBuilderUpdateUsageAndParse( - command: string | null, + isDefaultCommand: boolean, commandHandler: CommandHandler, yargs: YargsInstance, aliases: Dictionary, @@ -273,7 +274,7 @@ export class CommandInstance { return builderOutput.then(output => { innerYargs = isYargsInstance(output) ? output : yargs; return this.parseAndUpdateUsage( - command, + isDefaultCommand, commandHandler, innerYargs, parentCommands, @@ -291,7 +292,7 @@ export class CommandInstance { }); } return this.parseAndUpdateUsage( - command, + isDefaultCommand, commandHandler, innerYargs, parentCommands, @@ -300,7 +301,7 @@ export class CommandInstance { ); } private parseAndUpdateUsage( - command: string | null, + isDefaultCommand: boolean, commandHandler: CommandHandler, innerYargs: YargsInstance, parentCommands: string[], @@ -310,7 +311,8 @@ export class CommandInstance { // A null command indicates we are running the default command, // if this is the case, we should show the root usage instructions // rather than the usage instructions for the nested default command: - if (!command) innerYargs.getInternalMethods().getUsageInstance().unfreeze(); + if (isDefaultCommand) + innerYargs.getInternalMethods().getUsageInstance().unfreeze(); if (this.shouldUpdateUsage(innerYargs)) { innerYargs .getInternalMethods() @@ -357,7 +359,7 @@ export class CommandInstance { return `$0 ${pc.join(' ')}`; } private applyMiddlewareAndGetResult( - command: string | null, + isDefaultCommand: boolean, commandHandler: CommandHandler, innerArgv: Arguments | Promise, currentContext: Context, @@ -393,7 +395,7 @@ export class CommandInstance { aliases, positionalMap, (yargs.parsed as DetailedArguments).error, - !command + isDefaultCommand ); innerArgv = maybeAsyncResult(innerArgv, result => { validation(result); @@ -422,7 +424,10 @@ export class CommandInstance { } }); - yargs.getInternalMethods().getUsageInstance().cacheHelpMessage(); + if (!isDefaultCommand) { + yargs.getInternalMethods().getUsageInstance().cacheHelpMessage(); + } + if ( isPromise(innerArgv) && !yargs.getInternalMethods().hasParseCallback() @@ -438,7 +443,7 @@ export class CommandInstance { } } - if (command) { + if (!isDefaultCommand) { currentContext.commands.pop(); currentContext.fullCommands.pop(); } @@ -595,7 +600,7 @@ export class CommandInstance { }); } } - runDefaultBuilderOn(yargs: YargsInstance): void { + runDefaultBuilderOn(yargs: YargsInstance): unknown | Promise { if (!this.defaultCommand) return; if (this.shouldUpdateUsage(yargs)) { // build the root-level command string from the default string. @@ -609,12 +614,13 @@ export class CommandInstance { } const builder = this.defaultCommand.builder; if (isCommandBuilderCallback(builder)) { - builder(yargs, true); + return builder(yargs, true); } else if (!isCommandBuilderDefinition(builder)) { Object.keys(builder).forEach(key => { yargs.option(key, builder[key]); }); } + return undefined; } // lookup module object from require()d command and derive name // if module was not require()d and no name given, throw error diff --git a/lib/yargs-factory.ts b/lib/yargs-factory.ts index 592e678d7..983e1ccfa 100644 --- a/lib/yargs-factory.ts +++ b/lib/yargs-factory.ts @@ -779,6 +779,13 @@ export class YargsInstance { }); } } + // Ensure top level options/positionals have been configured: + const builderResponse = this.#command.runDefaultBuilderOn(this); + if (isPromise(builderResponse)) { + return builderResponse.then(() => { + return this.#usage.help(); + }); + } } return Promise.resolve(this.#usage.help()); } @@ -1258,6 +1265,14 @@ export class YargsInstance { return this; } } + // Ensure top level options/positionals have been configured: + const builderResponse = this.#command.runDefaultBuilderOn(this); + if (isPromise(builderResponse)) { + builderResponse.then(() => { + this.#usage.showHelp(level); + }); + return this; + } } this.#usage.showHelp(level); return this; @@ -2044,11 +2059,6 @@ export class YargsInstance { !!calledFromCommand, false ); - } else if (!calledFromCommand && helpOnly) { - // TODO(bcoe): what if the default builder is async? - // TODO(bcoe): add better comments for runDefaultBuilderOn, why - // are we doing this? - this.#command.runDefaultBuilderOn(this); } // we must run completions first, a user might @@ -2083,8 +2093,6 @@ export class YargsInstance { if (helpOptSet) { if (this.#exitProcess) setBlocking(true); skipValidation = true; - // TODO: add appropriate comment. - if (!calledFromCommand) this.#command.runDefaultBuilderOn(this); this.showHelp('log'); this.exit(0); } else if (versionOptSet) { diff --git a/package.json b/package.json index f22f81064..b72e1650b 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "yargs", - "version": "17.0.0-candidate.10", + "version": "17.0.0-candidate.12", "description": "yargs the modern, pirate-themed, successor to optimist.", "main": "./index.cjs", "exports": { diff --git a/test/usage.cjs b/test/usage.cjs index 03221ae3e..bea8bd35d 100644 --- a/test/usage.cjs +++ b/test/usage.cjs @@ -596,6 +596,7 @@ describe('usage tests', () => { // ignore the error, we only test the output here } }); + console.info(r); r.errors .join('\n') .split(/\n+/) @@ -4462,6 +4463,54 @@ describe('usage tests', () => { help.split('\n').should.deep.equal(expected); }); }); + // Refs: https://github.com/yargs/yargs/issues/1912 + describe('positional', () => { + const expected = [ + 'Hello, world!', + '', + 'Commands:', + ' usage [foo] Default command description [default]', + ' usage foo Foo command description', + '', + 'Positionals:', + ' foo foo parameter', + '', + 'Options:', + ' --help Show help [boolean]', + ' --version Show version number [boolean]', + ]; + it('should contain the expected output for --help', () => { + const r = checkUsage(() => + yargs('--help') + .scriptName('usage') + .usage('Hello, world!') + .command('* [foo]', 'Default command description', yargs => { + yargs.positional('foo', { + describe: 'foo parameter', + }); + }) + .commands([{command: 'foo', desc: 'Foo command description'}]) + .parse() + ); + r.logs[0].split('\n').should.deep.equal(expected); + }); + it('should contain the expected output for showHelp', () => { + const r = checkUsage(() => { + const y = yargs() + .scriptName('usage') + .usage('Hello, world!') + .command('* [foo]', 'Default command description', yargs => { + yargs.positional('foo', { + describe: 'foo parameter', + }); + }) + .commands([{command: 'foo', desc: 'Foo command description'}]); + y.parse(); + y.showHelp('log'); + }); + r.logs[0].split('\n').should.deep.equal(expected); + }); + }); }); describe('async builder', async () => { @@ -4502,6 +4551,56 @@ describe('usage tests', () => { logs.should.match(/a test command/); } }); + // Refs: https://github.com/yargs/yargs/issues/1912 + describe('positional', () => { + const expected = [ + 'Hello, world!', + '', + 'Commands:', + ' usage [foo] Default command description [default]', + ' usage foo Foo command description', + '', + 'Positionals:', + ' foo foo parameter', + '', + 'Options:', + ' --help Show help [boolean]', + ' --version Show version number [boolean]', + ]; + it('should contain the expected output for --help', async () => { + const r = await checkUsage(() => { + yargs('--help') + .scriptName('usage') + .usage('Hello, world!') + .command('* [foo]', 'Default command description', async yargs => { + await wait(); + yargs.positional('foo', { + describe: 'foo parameter', + }); + }) + .commands([{command: 'foo', desc: 'Foo command description'}]) + .parse(); + return wait(20); + }); + r.logs[0].split('\n').should.deep.equal(expected); + }); + it('should contain the expected output for getHelp', async () => { + const y = yargs() + .scriptName('usage') + .usage('Hello, world!') + .command('* [foo]', 'Default command description', async yargs => { + // await wait(); + yargs.positional('foo', { + describe: 'foo parameter', + }); + }) + .commands([{command: 'foo', desc: 'Foo command description'}]); + await y.parse(''); + await wait(); + const help = await y.getHelp(); + help.split('\n').should.deep.equal(expected); + }); + }); }); // Refs: https://github.com/yargs/yargs/issues/1820 diff --git a/test/yargs.cjs b/test/yargs.cjs index 5cf2b700f..f764ac1f5 100644 --- a/test/yargs.cjs +++ b/test/yargs.cjs @@ -2192,7 +2192,6 @@ describe('yargs dsl tests', () => { }) .coerce('option2', () => undefined) .getHelp(); - console.info(help); help.should.match(/option2 description/); }); });