fix: setting default: undefined causes implies to throw

[AgentEnder]

When setting .implies(key, otherKey), yargs will throw always throw a validation error if key has a default set, even if the default is undefined.

I do concede that setting an explicit default of undefined is a bit weird, but this is something I came across while setting up better validations in a codebase that already had yargs configured. In my case, I did just remove the default, but it seemed like something that shouldn't have necessarily been needed to change.

If you think this is unnecessary, I can close it out.

[shadowspawn]

I agree with your approach in principle. The author may have supplying a value they got from somewhere else and not have checked whether it is undefined. A familiar example is process.env.foo.

The keyExists routine has multiple blocks, and I think the `--no- should be updated to match the new logic.

I wonder about changing the routine name from keyExists with the change in implementation. Maybe keyDefined? Only change it if you think it is more descriptive/accurate!

[AgentEnder]

Hey, yeah that makes sense to me. Updated

[shadowspawn]

There is a negation on this one, so:

Suggested change

	val =
	!Object.prototype.hasOwnProperty.call(argv, val) &&
	argv[val] !== undefined;
	val =
	!Object.prototype.hasOwnProperty.call(argv, val) ||
	argv[val] === undefined;

[AgentEnder]

Ah, good catch.

[shadowspawn]

LGTM

[jly36963]

A few concerns:

• implies and conflicts should probably have similar behavior around undefined. If implies can now be bypassed by using undefined, conflicts should match, right?
• The documentation for implies and conflicts specifies the keys being the criteria for these validations, not the values (or whether or not they are nullish/undefined/etc). If this change is merged, I would like to see the documentation updated.
• IMO, this change is a breaking change. People might be very technical about the keys being the criteria that these validations throw on, and that will no longer be the case. This package gets >60 million weekly downloads, I would tread carefully around these kind of changes.

In order to maintain the current behavior, could there be a configuration option to opt into the new behavior, or maybe a custom check/middleware implemented in order to achieve the desired behavior?

[shadowspawn]

Thanks for comments @jly36963 . It looks like conflicts may ignore undefined already, albeit possibly for different reasons from the comments. I found this in the code, but I have not verified the runtime:

https://github.com/yargs/yargs/blame/e517318cea0087b813f5de414b3cdec7b70efe33/lib/validation.ts#L406

In order to maintain the current behavior, could there be a configuration option to opt into the new behavior, or maybe a custom check/middleware implemented in order to achieve the desired behavior?

No, I do not think this is worth separate configuration. Any discussion about a "defined" value of undefined gets a bit meta! My reasoning for considering ignoring undefined is that the cases where it occurs are often when there is not a value, like an undefined environment variable or accessing missing array element.

[jly36963]

Yeah, that might already be the case with conflicts, I'll try to verify this behavior today. That might just be an unfortunate side effect of not effectively separating internal logic from user-provided values. (For example, version is a yargs method that collides with 'version' as an option, and that lack of separation lead to several people reporting unexpected behavior.)

---

I still think switching the implies logic from focusing on keys to values will break some existing users' programs, and if this change is acceptable I think updating the docs would be co-requisite.

[jly36963]

Another simple solution might be to use a pre-validation middleware that removes keys from argv if their values are undefined

[shadowspawn]

Looking through the the documentation, I spotted one case where Yargs sets an option to undefined when it is used on the command-line. I think this means we can't change the treatment of undefined per this PR after all, as while arguably improves one edge case it alters (at least one) other edge cases.

• https://yargs.js.org/docs/#api-reference-numberkey

If the option is given on the command line without a value, argv will be populated with undefined.

I tried it out with a toy program, and sure enough this PR changes the behaviour:

console.log(require('yargs')(process.argv.slice(2))
    .number('num', 'description')
    .string('order')
    .implies({ num:'order'})
    .argv)

# Before PR
$ node index.js --num
Options:
  --help     Show help                                                 [boolean]
  --version  Show version number                                       [boolean]
  --num                                                                 [number]
  --order                                                               [string]

Missing dependent arguments:
 num -> order

# After PR
$ node index.js --num
{ _: [], num: undefined, '$0': 'index.js' }

[jly36963]

Generally, the reason I am so hesitant with this change is related to my use of another library.

With Knex (sql querybuilder), both inserts/updates ignore keys with undefined values and treat keys with null values as null. I rely on this behavior remaining unchanged. If it ever did change, that would break my (and likely several other) applications.

I imagine that there are people who rely on yargs behavior remaining unchanged, in the same way that I rely on Knex behavior remaining unchanged.

I recognize that Knex's behavior around keys/undefined is similar to the behavior that this PR is trying to faciliate, so it is (of sorts) the inverse scenario. In other words, changing the behavior around keys with undefined values:

• leveraging -> ignoring
• ignoring -> leveraging

Nonetheless, I anticipate people being affected by this change

[shadowspawn]

Going to close this as not-safe-enough, sorry @AgentEnder , thanks for the PR.

It can be hard to fix edge cases in a big complex program without risking affecting other cases.