From d0e829239580bd44873bbde65de2ed7671aa2ab0 Mon Sep 17 00:00:00 2001 From: Landon Yarrington <33426811+jly36963@users.noreply.github.com> Date: Sun, 5 Sep 2021 17:09:25 -0600 Subject: [PATCH] fix: emit warning on version name collision (#1986) --- lib/platform-shims/browser.mjs | 1 + lib/platform-shims/cjs.ts | 2 ++ lib/platform-shims/deno.ts | 1 + lib/platform-shims/esm.mjs | 3 ++- lib/typings/common-types.ts | 1 + lib/yargs-factory.ts | 30 +++++++++++++++++++++ test/helpers/utils.cjs | 7 +++++ test/usage.cjs | 49 ++++++++++++++++++++++++++++++++++ 8 files changed, 93 insertions(+), 1 deletion(-) diff --git a/lib/platform-shims/browser.mjs b/lib/platform-shims/browser.mjs index 5bba14653..5f8ec61f4 100644 --- a/lib/platform-shims/browser.mjs +++ b/lib/platform-shims/browser.mjs @@ -41,6 +41,7 @@ export default { process: { argv: () => [], cwd: () => '', + emitWarning: (warning, name) => {}, execPath: () => '', // exit is noop browser: exit: () => {}, diff --git a/lib/platform-shims/cjs.ts b/lib/platform-shims/cjs.ts index 03f36f797..6a18223c2 100644 --- a/lib/platform-shims/cjs.ts +++ b/lib/platform-shims/cjs.ts @@ -26,6 +26,8 @@ export default { process: { argv: () => process.argv, cwd: process.cwd, + emitWarning: (warning: string | Error, type?: string) => + process.emitWarning(warning, type), execPath: () => process.execPath, exit: (code: number) => { // eslint-disable-next-line no-process-exit diff --git a/lib/platform-shims/deno.ts b/lib/platform-shims/deno.ts index 0c4436bfe..6bea0422b 100644 --- a/lib/platform-shims/deno.ts +++ b/lib/platform-shims/deno.ts @@ -82,6 +82,7 @@ export default { process: { argv: () => argv, cwd: () => cwd, + emitWarning: (warning: string | Error, type?: string) => {}, execPath: () => { try { return Deno.execPath(); diff --git a/lib/platform-shims/esm.mjs b/lib/platform-shims/esm.mjs index bc0479162..f81f7ea4e 100644 --- a/lib/platform-shims/esm.mjs +++ b/lib/platform-shims/esm.mjs @@ -3,7 +3,7 @@ import { notStrictEqual, strictEqual } from 'assert' import cliui from 'cliui' import escalade from 'escalade/sync' -import { format, inspect } from 'util' +import { inspect } from 'util' import { readFileSync } from 'fs' import { fileURLToPath } from 'url'; import Parser from 'yargs-parser' @@ -45,6 +45,7 @@ export default { process: { argv: () => process.argv, cwd: process.cwd, + emitWarning: (warning, type) => process.emitWarning(warning, type), execPath: () => process.execPath, exit: process.exit, nextTick: process.nextTick, diff --git a/lib/typings/common-types.ts b/lib/typings/common-types.ts index 9b9bcfcda..10e44d5ec 100644 --- a/lib/typings/common-types.ts +++ b/lib/typings/common-types.ts @@ -103,6 +103,7 @@ export interface PlatformShim { process: { argv: () => string[]; cwd: () => string; + emitWarning: (warning: string | Error, type?: string) => void; execPath: () => string; exit: (code: number) => void; nextTick: (cb: Function) => void; diff --git a/lib/yargs-factory.ts b/lib/yargs-factory.ts index 23d6e0f27..7c39e2883 100644 --- a/lib/yargs-factory.ts +++ b/lib/yargs-factory.ts @@ -88,6 +88,7 @@ export function YargsFactory(_shim: PlatformShim) { const kCopyDoubleDash = Symbol('copyDoubleDash'); const kCreateLogger = Symbol('copyDoubleDash'); const kDeleteFromParserHintObject = Symbol('deleteFromParserHintObject'); +const kEmitWarning = Symbol('emitWarning'); const kFreeze = Symbol('freeze'); const kGetDollarZero = Symbol('getDollarZero'); const kGetParserConfiguration = Symbol('getParserConfiguration'); @@ -172,6 +173,7 @@ export class YargsInstance { #defaultShowHiddenOpt = 'show-hidden'; #exitError: YError | string | undefined | null = null; #detectLocale = true; + #emittedWarnings: Dictionary = {}; #exitProcess = true; #frozens: FrozenYargsInstance[] = []; #globalMiddleware: GlobalMiddleware; @@ -904,6 +906,23 @@ export class YargsInstance { opt = {}; } + // Warn about version name collision + // Addresses: https://github.com/yargs/yargs/issues/1979 + if (this.#versionOpt && (key === 'version' || opt?.alias === 'version')) { + this[kEmitWarning]( + [ + '"version" is a reserved word.', + 'Please do one of the following:', + '- Disable version with `yargs.version(false)` if using "version" as an option', + '- Use the built-in `yargs.version` method instead (if applicable)', + '- Use a different option key', + 'https://yargs.js.org/docs/#api-reference-version', + ].join('\n'), + undefined, + 'versionWarning' // TODO: better dedupeId + ); + } + this.#options.key[key] = true; // track manually set keys. if (opt.alias) this.alias(key, opt.alias); @@ -1449,6 +1468,17 @@ export class YargsInstance { // now delete the description from usage.js. delete this.#usage.getDescriptions()[optionKey]; } + [kEmitWarning]( + warning: string, + type: string | undefined, + deduplicationId: string + ) { + // prevent duplicate warning emissions + if (!this.#emittedWarnings[deduplicationId]) { + this.#shim.process.emitWarning(warning, type); + this.#emittedWarnings[deduplicationId] = true; + } + } [kFreeze]() { this.#frozens.push({ options: this.#options, diff --git a/test/helpers/utils.cjs b/test/helpers/utils.cjs index 8d8eef723..d4e6ca30e 100644 --- a/test/helpers/utils.cjs +++ b/test/helpers/utils.cjs @@ -9,6 +9,7 @@ exports.checkOutput = function checkOutput(f, argv, cb) { let exitCode = 0; const _exit = process.exit; const _emit = process.emit; + const _emitWarning = process.emitWarning; const _env = process.env; const _argv = process.argv; const _error = console.error; @@ -25,6 +26,7 @@ exports.checkOutput = function checkOutput(f, argv, cb) { const errors = []; const logs = []; const warnings = []; + const emittedWarnings = []; console.error = (...msg) => { errors.push(format(...msg)); @@ -35,6 +37,9 @@ exports.checkOutput = function checkOutput(f, argv, cb) { console.warn = (...msg) => { warnings.push(format(...msg)); }; + process.emitWarning = warning => { + emittedWarnings.push(warning); + }; let result; @@ -80,6 +85,7 @@ exports.checkOutput = function checkOutput(f, argv, cb) { process.emit = _emit; process.env = _env; process.argv = _argv; + process.emitWarning = _emitWarning; console.error = _error; console.log = _log; @@ -93,6 +99,7 @@ exports.checkOutput = function checkOutput(f, argv, cb) { errors, logs, warnings, + emittedWarnings, exit, exitCode, result, diff --git a/test/usage.cjs b/test/usage.cjs index 24b29476b..eef18facc 100644 --- a/test/usage.cjs +++ b/test/usage.cjs @@ -5,6 +5,7 @@ const checkUsage = require('./helpers/utils.cjs').checkOutput; const chalk = require('chalk'); const yargs = require('../index.cjs'); +const expect = require('chai').expect; const {YError} = require('../build/index.cjs'); const should = require('chai').should(); @@ -1570,6 +1571,54 @@ describe('usage tests', () => { r.logs[0].should.eql('1.0.2'); }); + // Addresses: https://github.com/yargs/yargs/issues/1979 + describe('when an option or alias "version" is set', () => { + it('emits warning if version is not disabled', () => { + const r = checkUsage(() => + yargs + .command('cmd1', 'cmd1 desc', yargs => + yargs.option('v', { + alias: 'version', + describe: 'version desc', + type: 'string', + }) + ) + .fail(() => { + expect.fail(); + }) + .wrap(null) + .parse('cmd1 --version 0.25.10') + ); + r.should.have.property('emittedWarnings').with.length(1); + r.emittedWarnings[0].should.match(/reserved word/); + }); + + it('does not emit warning if version is disabled', () => { + const r = checkUsage(() => + yargs + .command( + 'cmd1', + 'cmd1 desc', + yargs => + yargs.version(false).option('version', { + alias: 'v', + describe: 'version desc', + type: 'string', + }), + argv => { + argv.version.should.equal('0.25.10'); + } + ) + .fail(() => { + expect.fail(); + }) + .parse('cmd1 --version 0.25.10') + ); + + r.should.have.property('emittedWarnings').with.length(0); + }); + }); + describe('when exitProcess is false', () => { it('should not validate arguments (required argument)', () => { const r = checkUsage(() =>