Skip to content

Commit

Permalink
fix: emit warning on version name collision (#1986)
Browse files Browse the repository at this point in the history
  • Loading branch information
jly36963 committed Sep 5, 2021
1 parent 01b2c6a commit d0e8292
Show file tree
Hide file tree
Showing 8 changed files with 93 additions and 1 deletion.
1 change: 1 addition & 0 deletions lib/platform-shims/browser.mjs
Expand Up @@ -41,6 +41,7 @@ export default {
process: {
argv: () => [],
cwd: () => '',
emitWarning: (warning, name) => {},
execPath: () => '',
// exit is noop browser:
exit: () => {},
Expand Down
2 changes: 2 additions & 0 deletions lib/platform-shims/cjs.ts
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions lib/platform-shims/deno.ts
Expand Up @@ -82,6 +82,7 @@ export default {
process: {
argv: () => argv,
cwd: () => cwd,
emitWarning: (warning: string | Error, type?: string) => {},
execPath: () => {
try {
return Deno.execPath();
Expand Down
3 changes: 2 additions & 1 deletion lib/platform-shims/esm.mjs
Expand Up @@ -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'
Expand Down Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions lib/typings/common-types.ts
Expand Up @@ -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;
Expand Down
30 changes: 30 additions & 0 deletions lib/yargs-factory.ts
Expand Up @@ -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');
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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,
Expand Down
7 changes: 7 additions & 0 deletions test/helpers/utils.cjs
Expand Up @@ -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;
Expand All @@ -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));
Expand All @@ -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;

Expand Down Expand Up @@ -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;
Expand All @@ -93,6 +99,7 @@ exports.checkOutput = function checkOutput(f, argv, cb) {
errors,
logs,
warnings,
emittedWarnings,
exit,
exitCode,
result,
Expand Down
49 changes: 49 additions & 0 deletions test/usage.cjs
Expand Up @@ -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();
Expand Down Expand Up @@ -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(() =>
Expand Down

0 comments on commit d0e8292

Please sign in to comment.