null prototype for returned values?

[shadowspawn]

(I want a separate issue from #106, which is otherwise transparent improvements that do not affect the user experience.)

There are two potential benefits to using a null-prototype for values:

• can use otherwise problematic option names like toString. (Although the usual convention for CLI is kebab-case like to-string which bypasses the issue.)
• somewhat safer use in our code

The potential downside is confusing authors. Examples:

• Custom and Null objects
• https://stackoverflow.com/questions/56298481/how-to-fix-object-null-prototype-title-product

Related discussions:

• Paranoia with Prototype Pollution #32 (comment)
• Paranoia with null object #33
• util: add parseArgs module nodejs/node#42675 (comment)

(Paraphrased from one of discussions.) For comparison, by default Commander uses plain {} for storing the option values. I did also consider Map and null prototype at the time: tj/commander.js#1102 (comment))

[ljharb]

Null prototypes are a basic part of JavaScript at this point; if someone’s confused by console output that has it, then it’s a learning opportunity for them.

[bakkot]

There's precedent for this in the language (e.g. the groups property of regex objects), in Node core (e.g. querystring.parse), and userland (e.g. graphql).

[shadowspawn]

Thanks @bakkot . The examples are good, and the querystring.parse example is great. I am reassured.

I was wondering about a mention in the documentation and see there is for querystring.parse.

[shadowspawn]

Other examples from @ljharb in nodejs/node#42675 (comment)

I don't think they're that rare - every import * as foo is one, as is every .groups on a RegExp match object.

[shadowspawn]

somewhat safer use in our code

I wasn't actually expecting a case to turn up, let alone so quickly!

• fix: resist pollution #106 (comment)

[shadowspawn]

nodejs/node#42675 (comment)

I'd be interested to see any userland code - historical or modern - for which this would impact it in a negative way.

Cough. Maybe extra code needed for unit tests of results. Found the hard way. 😓

[shadowspawn]

Another reference of stumbles, this time looking for graphql issues:

• https://stackoverflow.com/questions/53983315/is-there-a-way-to-get-rid-of-object-null-prototype-in-graphql/68503030#68503030

Top rated answer is:

const a = JSON.parse(JSON.stringify(args));

[ljharb]

or simply Object.assign({}, nullObject) or { ...nullObject }. JSON roundtripping breaks a ton of JS values, and should be avoided in most cases.

[shadowspawn]

Yes, I cried a little when I saw the JSON answer again. 😢

[shadowspawn]

Interesting, JSON is actually given as the example mechanism for deep copy, so not as random as I thought:

• https://developer.mozilla.org/en-US/docs/Glossary/Deep_copy

[targos]

structuredClone({__proto__: null}) also creates an object with Object.prototype.

[shadowspawn]

Took me a while to work out structuredClone was added in node 17.

[shadowspawn]

Having hit problems requiring time and effort to research to do something users may encounter, I am back to ambivalent about using null prototype. 😞

[ljharb]

Object spread is The Way to shallow copy objects, and it’s what most users will reach for. I don’t think this is an actual problem.

nonzero users are going to be confused no matter what we do; we should just do the safest thing and document well.

[shadowspawn]

In previous discussions (#33) we were wondering about using a Map or null prototype internally, but returning a normal object. I have minimal concerns about doing that.

#33 (comment)

[shadowspawn]

Object spread is The Way to shallow copy objects, and it’s what most users will reach for. I don’t think this is an actual problem.

This is quite a good example of the complications. Yes, object spread is pretty awesomely useful for many situations. Likewise we also have Object.assign() offered earlier (#108 (comment)).

But we are returning a null prototype object inside the result. The case I hit and showed in the PR is not fixed by shallow copy of the result.

I launched into null prototype after it being suggested a second time and optimistic about the trade-offs. And it promptly blew up. So I'm pushing back and reevaluating the tradeoffs!

[ljharb]

I don't expect people to use the result object directly; i expect them to destructure it immediately.

[shadowspawn]

Far from convinced that null objects won't trip up multiple people, but done! Which makes null prototype one small step closer to routine...

[aaronccasanova]

Far from convinced that null objects won't trip up multiple people

Agreed. I'm impartial either way, but do think this could be a slight annoyance to users expecting access to prototype methods (values.hasOwnProperty). Glad I caught this thread with all the nifty workarounds!

I don't expect people to use the result object directly; i expect them to destructure it immediately.

I have only been destructuring in examples and tests for conciseness. However, in production I will likely always assign the result object directly to a variable (e.g. const cli = parseArgs({ options })). Unless renamed, I will definitely forget what values is when I'm hundreds of lines deep in a real world script (as opposed to cli.values). To your actual point though, I had a play with all the workarounds suggested in this thread (e.g. {...cli.values}, Object.assign(), structuredClone) and am not concerned with us using null prototypes. In addition to our own documentation, the errors are super quick to identify and resolve with a simple stack overflow.