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
15 changes: 15 additions & 0 deletions lib/yargs-factory.ts
Expand Up @@ -904,6 +904,21 @@ export class YargsInstance {
opt = {};
}

// Prevent version name collision
// Addresses: https://github.com/yargs/yargs/issues/1979
if (this.#versionOpt && (key === 'version' || opt?.alias === 'version')) {
throw new YError(
[
'"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')
);
}

this.#options.key[key] = true; // track manually set keys.

if (opt.alias) this.alias(key, opt.alias);
Expand Down
42 changes: 42 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,47 @@ 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('fails unless version is disabled', done => {
yargs
.command('cmd1', 'cmd desc', yargs =>
yargs.option('v', {
alias: 'version',
describe: 'version desc',
type: 'string',
})
)
.fail(msg => {
msg.should.match(/reserved word/);
return done();
})
.wrap(null)
.parse('cmd1 --version 0.25.10');
});

it('works if version is disabled', () => {
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');
});
});

describe('when exitProcess is false', () => {
it('should not validate arguments (required argument)', () => {
const r = checkUsage(() =>
Expand Down