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: coerce pollutes argv #2161

Merged
merged 5 commits into from Apr 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/command.ts
Expand Up @@ -603,9 +603,9 @@ export class CommandInstance {
// If both positionals/options provided, no default was set,
// and if at least one is an array: don't overwrite, combine.
if (
!Object.prototype.hasOwnProperty.call(defaults, key) &&
Object.prototype.hasOwnProperty.call(argv, key) &&
Object.prototype.hasOwnProperty.call(parsed.argv, key) &&
!Object.hasOwnProperty.call(defaults, key) &&
Object.hasOwnProperty.call(argv, key) &&
Object.hasOwnProperty.call(parsed.argv, key) &&
(Array.isArray(argv[key]) || Array.isArray(parsed.argv[key]))
) {
argv[key] = ([] as string[]).concat(argv[key], parsed.argv[key]);
Expand Down
12 changes: 11 additions & 1 deletion lib/yargs-factory.ts
Expand Up @@ -387,6 +387,13 @@ export class YargsInstance {
yargs: YargsInstance
): Partial<Arguments> | Promise<Partial<Arguments>> => {
let aliases: Dictionary<string[]>;

// Skip coerce logic if related arg was not provided
const shouldCoerce = Object.hasOwnProperty.call(argv, keys);
if (!shouldCoerce) {
return argv;
}

return maybeAsyncResult<
Partial<Arguments> | Promise<Partial<Arguments>> | any
>(
Expand All @@ -396,7 +403,10 @@ export class YargsInstance {
},
(result: any): Partial<Arguments> => {
argv[keys] = result;
if (aliases[keys]) {
const stripAliased = yargs
.getInternalMethods()
.getParserConfiguration()['strip-aliased'];
if (aliases[keys] && stripAliased !== true) {
for (const alias of aliases[keys]) {
argv[alias] = result;
}
Expand Down
53 changes: 53 additions & 0 deletions test/command.cjs
Expand Up @@ -1647,6 +1647,59 @@ describe('Command', () => {
argv.rest.should.equal('bar baz');
coerceExecutionCount.should.equal(1);
});

// Addresses: https://github.com/yargs/yargs/issues/2130
it('should not run or set new properties on argv when related argument is not passed', () => {
yargs('cmd1')
.command(
'cmd1',
'cmd1 desc',
yargs =>
yargs
.option('foo', {alias: 'f', type: 'string'})
.option('bar', {
alias: 'b',
type: 'string',
implies: 'f',
coerce: () => expect.fail(), // Should not be called
})
.fail(() => {
expect.fail(); // Should not fail because of implies
}),
argv => {
// eslint-disable-next-line no-prototype-builtins
if (Object.prototype.hasOwnProperty(argv, 'b')) {
expect.fail(); // 'b' was not provided, coerce should not set it
}
}
)
.strict()
.parse();
});

// Addresses: https://github.com/yargs/yargs/issues/2159
it('should not add aliases to argv if strip-aliased config is true', () => {
yargs('cmd1 -f hello -b world')
.parserConfiguration({'strip-aliased': true})
.command(
'cmd1',
'cmd1 desc',
yargs =>
yargs.option('foo', {alias: 'f', type: 'string'}).option('bar', {
type: 'string',
alias: 'b',
coerce: val => val,
}),
argv => {
// eslint-disable-next-line no-prototype-builtins
if (Object.prototype.hasOwnProperty(argv, 'b')) {
expect.fail(); // 'b' is an alias, it should not be in argv
}
}
)
.strict()
.parse();
});
});

describe('defaults', () => {
Expand Down