Repository navigation
null prototype for returned values? #108
Description
(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 liketo-stringwhich 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))
Activity
ljharb commentedon Apr 14, 2022
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 commentedon Apr 14, 2022
There's precedent for this in the language (e.g. the
groupsproperty of regex objects), in Node core (e.g. querystring.parse), and userland (e.g. graphql).shadowspawn commentedon Apr 14, 2022
Thanks @bakkot . The examples are good, and the
querystring.parseexample is great. I am reassured.I was wondering about a mention in the documentation and see there is for
querystring.parse.shadowspawn commentedon Apr 14, 2022
Other examples from @ljharb in nodejs/node#42675 (comment)
shadowspawn commentedon Apr 15, 2022
I wasn't actually expecting a case to turn up, let alone so quickly!
shadowspawn commentedon Apr 15, 2022
nodejs/node#42675 (comment)
Cough. Maybe extra code needed for unit tests of results. Found the hard way. 😓
shadowspawn commentedon Apr 15, 2022
Another reference of stumbles, this time looking for graphql issues:
Top rated answer is:
ljharb commentedon Apr 15, 2022
or simply
Object.assign({}, nullObject)or{ ...nullObject }. JSON roundtripping breaks a ton of JS values, and should be avoided in most cases.shadowspawn commentedon Apr 15, 2022
Yes, I cried a little when I saw the JSON answer again. 😢
shadowspawn commentedon Apr 15, 2022
Interesting,
JSONis actually given as the example mechanism for deep copy, so not as random as I thought:targos commentedon Apr 15, 2022
structuredClone({__proto__: null})also creates an object with Object.prototype.shadowspawn commentedon Apr 15, 2022
Took me a while to work out
structuredClonewas added in node 17.shadowspawn commentedon Apr 15, 2022
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 commentedon Apr 15, 2022
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 commentedon Apr 15, 2022
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 commentedon Apr 15, 2022
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 commentedon Apr 15, 2022
I don't expect people to use the result object directly; i expect them to destructure it immediately.
shadowspawn commentedon Apr 16, 2022
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 commentedon Apr 16, 2022
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 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 whatvaluesis when I'm hundreds of lines deep in a real world script (as opposed tocli.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 usingnullprototypes. In addition to our own documentation, the errors are super quick to identify and resolve with a simple stack overflow.