Closed shadowspawn closed 2 years ago
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.
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).
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
.
Other examples from @ljharb in https://github.com/nodejs/node/pull/42675#discussion_r849899245
I don't think they're that rare - every
import * as foo
is one, as is every.groups
on a RegExp match object.
somewhat safer use in our code
I wasn't actually expecting a case to turn up, let alone so quickly!
https://github.com/nodejs/node/pull/42675#discussion_r849922051
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. 😓
Another reference of stumbles, this time looking for graphql issues:
Top rated answer is:
const a = JSON.parse(JSON.stringify(args));
or simply Object.assign({}, nullObject)
or { ...nullObject }
. JSON roundtripping breaks a ton of JS values, and should be avoided in most cases.
Yes, I cried a little when I saw the JSON answer again. 😢
Interesting, JSON
is actually given as the example mechanism for deep copy, so not as random as I thought:
structuredClone({__proto__: null})
also creates an object with Object.prototype.
Took me a while to work out structuredClone
was added in node 17.
Having hit problems requiring time and effort to research to do something users may encounter, I am back to ambivalent about using null prototype. 😞
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.
In previous discussions (https://github.com/pkgjs/parseargs/issues/33) we were wondering about using a Map or null prototype internally, but returning a normal object. I have minimal concerns about doing that.
https://github.com/pkgjs/parseargs/issues/33#issuecomment-1013916209
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 (https://github.com/pkgjs/parseargs/issues/108#issuecomment-1099901622).
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!
I don't expect people to use the result object directly; i expect them to destructure it immediately.
Far from convinced that null objects won't trip up multiple people, but done! Which makes null prototype one small step closer to routine...
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.
(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
:toString
. (Although the usual convention for CLI is kebab-case liketo-string
which bypasses the issue.)The potential downside is confusing authors. Examples:
Related discussions:
(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: https://github.com/tj/commander.js/pull/1102#issue-527469955)