Propose stop storing explicit undefined in values for flags

[shadowspawn]

I have touched on this in a few different issues. Opening a separate issue to make this visible and separately identified as a suggestion (and discussion). Note: I used the proposed behaviour in all the examples in #38.

I do not think storing an explicit undefined in values when flags are parsed is adding any value (pun intended).

const { parseArgs } = require('@pkgjs/parseargs');
const parsed = parseArgs(['--foo']);
console.log(parsed);

Current behaviour:

{ flags: { foo: true }, values: { foo: undefined }, positionals: [] }

Proposed behaviour:

{ flags: { foo: true }, values: {}, positionals: [] }

[ljharb]

I think it's far better to have the explicit undefined, so that it works with reflection mechanisms (Object.keys/values/entries, Reflect.ownKeys, etc), and so Reflect.hasOwn gives an unsurprising answer.

[shadowspawn]

The proposed/original "existence" pattern has two objects in the results. I have been calling them options and values (see #38). The dual objects is a core part of the design, and different than other libraries.

options is the natural one to iterate over if you want all the options, whether used as a flag or with a value.

Why is it better to have reflection claim that there is an entry in values, but then the value turns out to be undefined?

[ljharb]

Right - I'm talking about the cases where I don't want all the options, i only want the flags, or, only the values.

[shadowspawn]

Exploring the parse results...

// const parsed = parseArgs(['--foo=123', '--bar'], { withValue: ['foo'] });

const parsed = {
    options: { foo: true, bar: true },
    values: { foo: '123' },
    positionals: []
};

console.log('Used with value 1:');
for (const option in parsed.values) {
    console.log(`${option}: ${parsed.values[option]}`);
}

console.log('\nUsed with value 2:');
console.log(parsed.values);
  
console.log('\nUsed as flag 1:');
const flags = Object.keys(parsed.options).filter((option) => parsed.values[option] === undefined);
console.log(flags);

console.log('\nUsed as flag 2:');
for (const option in parsed.options) {
    if (parsed.values[option] === undefined)
        console.log(option);
}

% node one-or-other.js
Used with value 1:
foo: 123

Used with value 2:
{ foo: '123' }

Used as flag 1:
[ 'bar' ]

Used as flag 2:
bar

[ljharb]

for-in is a bad practice, that's been discouraged for decades - we should be sure we don't use it in any examples or documentation.

I understand that if you use options as your source of truth always, then the absence or presence of undefined values isn't important. However, if you want to use values as your source of truth, the absence hurts. How does it hurt to have them present?

[shadowspawn]

for-in is a bad practice, that's been discouraged for decades - we should be sure we don't use it in any examples or documentation.

Ha, serves me right for quickly grabbing a pattern off the internet to do something "normal" with the results. 😊

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in

I understand that if you use options as your source of truth always, then the absence or presence of undefined values isn't important. However, if you want to use values as your source of truth, the absence hurts. How does it hurt to have them present?

I find it a cognitive load to deal with properties which may be implicitly or explicitly undefined. I have to carefully think about what question I am asking. When I iterate through the properties, the property values may be undefined despite being "defined". I find it detracts from working with the values, there is now a mixture of "real" values from the command-line along with these undefined, which I see when I examine the results and read the examples.

The defined undefined is a subtle situation that I struggled to work with when I first came across a situation where it mattered, and would prefer to avoid.

[ljharb]

That’s just an inherent part of the language. It’s not something API designers can fix - a property can be present or absent, and if present, undefined is one of the possible values. Just because NaN has typeof number doesn’t mean one should avoid using NaN when appropriate :-)

[shadowspawn]

When appropriate. I am proposing the change because I think the API is better without it.

[ljharb]

You've provided examples of how a present-undefined may not be needed. Can you provide any examples where having it causes problems?

[shadowspawn]

const { parseArgs } = require('@pkgjs/parseargs');

const withValue = ['warp-speed', 'destination'];
const result = parseArgs(undefined, { withValue });

// Custom behaviour, error for empty strings
for (let [key, value] of Object.entries(result.values)) {
    if (value.length === 0) {
        console.error(`option value can not be empty: --${key}`);
        process.exit(2);
    }
}
console.log('To infinity and beyond...')

% node problem.js --warp-speed=
option value can not be empty: --warp-speed

% node problem.js --warp-speed=9 --destination earth
To infinity and beyond...

% node problem.js --warp-speed=9 --destination earth --stealth
/Users/john/Documents/Sandpits/pkgjs/parseargs-issues/play/problem.js:8
    if (value.length === 0) {
              ^

TypeError: Cannot read properties of undefined (reading 'length')
...

[ljharb]

Sure, but in the world we're talking about, value isn't only a string, so that's just a bug. The diff to resolve your problem is either:

-if (value.length === 0) {
+if (value?.length === 0) {

or:

-if (value.length === 0) {
+if (!value) {

or a number of other alternatives.

[shadowspawn]

Sure, but bugs are problems.

[shadowspawn]

You said:

I think it's far better to have the explicit undefined, so that it works with reflection mechanisms (Object.keys/values/entries, Reflect.ownKeys, etc), and so Reflect.hasOwn gives an unsurprising answer.

And I asked:

Why is it better to have reflection claim that there is an entry in values, but then the value turns out to be undefined?

I am still not seeing the "far better"?

[ljharb]

Contrived bugs can happen in either case; I'm looking particularly at which use cases are made unacceptably difficult by one choice or the other.

The language differentiates absence vs present+undefined, and many things care about this - object rest/spread/Object.assign; all of the reflection methods (keys/values/entries/getOwnPropertyDescriptors/ownKeys); the stage 1 pattern matching proposal; etc. undefined is a value like anything else, and shouldn't be special-cased in a surprising way.

[bcoe]

@shadowspawn perhaps my opinion will change, but as I experiment with porting the yargs-parser DSL to parseArgs, I'm not finding undefined being set in values too painful -- perhaps this opinion will change once I get deeper into the project.

[shadowspawn]

I found it less painful when I realised I didn't need to use it, which made me question why have it. Have you used the storage of undefined or blissfully ignored it?

I'm considering a long comment with some research. I am still thinking it is wrong semantically, and may cause breaking changes for possible future additions (e.g. defaults).

[shadowspawn]

Only got comments from one person on this (thanks @ljharb). Closing in favour of #70 with wider proposal and context.

[shadowspawn]

Oops, and a comment from @bcoe, thanks Benjamin too. Scrolled too quickly. 🙈