Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prevent version name collision #1986

Merged
merged 15 commits into from Sep 5, 2021
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:',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think throwing an error is too heavy handed, as it would break a lot of applications. Here's what I would do:

  1. emit a custom warning with a similar message to this, using the approach described here. We should make sure that we don't show it more than once, using the approach outlined in the docs.
  2. I think we should turn off our own handling of .version() if someone configures it themselves, which the warning message can draw attention to.

What I'm not sure of, is whether we should call this a breaking change.

Copy link
Contributor Author

@jly36963 jly36963 Aug 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Questions

  • Would emitting a warning be sufficient? It would keep this from becoming a breaking change.
  • I left process.emitWarning as a noop in the deno shim. Is there a deno equivalent that I can use instead?
  • I left type and deduplicationID separate, so that process.emitWarning can be called without a type, but still prevent duplicate warnings. Does that make sense?
  • I didn't know what values to use for type/deduplicationID -- can I get your suggestions?

'- 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