fix: emit warning on version name collision (#1986)

jly36963 · web-flow · commit d0e829239580 · 2021-09-05T16:09:25.000-07:00
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<boolean> = {};
   #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
@@ -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
@@ -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(() =>
