doc: refine sample code on tokens of util/parseArgs

[imcotton]

Reduce mutations / side-effects on the code snippet in docs.

[shadowspawn]

I don't have a strong opinion, just a reaction.

The example is about using tokens. The old example code does not destructure much, and you see the tokens array and the token elements getting obviously named and processed, like both lines of:

tokens
   .filter((token) => token.kind === 'option')

That might be a positive rather than a negative for the purposes of a tokens example.

(Disclaimer: I wrote the example.)

[imcotton]

tokens
   .filter((token) => token.kind === 'option')

Thank you to bring this up, I totally agree to have the token named to increase readability especially for sake of documentation purpose, I will change it back soon. (Done)

However for case of .reduce(...) below, without destructuring would otherwise too verbose (5 places) to me even with documentation purpose in mind, would it be OK to leave it as, or consistency is more important here? (waiting feedback...)

[bakkot]

Personally I find this snippet significantly harder to follow than the original, which should be a high priority for documentation. So I'm -1 on this change (as a contributor to parseArgs, but not a node maintainer).

The original also has the advantage of being O(n) rather than O(n^2); if your concern is purely about best practices, you should definitely find a way to do this without a quadratic algorithm.

[imcotton]

To my knowledge, the delete operator would causing v8 de-optimize on its hidden class and turning it into slow object, it also makes TypeScript yelling:

The operand of a 'delete' operator must be optional.   ts(2790)

---

Performance checklist:

• How may times would parseArgs runs? once
• How may args should one program be taking? less than 10
• Have you done any measurement? no

are you looking for Premature Optimization?

[bakkot]

I'm not saying that it will actually be slow enough to matter in this instance. I'm saying that if you want to encourage good practices in general, you should not have a sample where there is a reduce which makes a copy of the whole structure, because that's quadratic.

If your concern is not about what general practices this code is fostering, I don't understand why you're suggesting a change at all. All the PR description says is "avoid mutations / side-effects", which is a principle some people like to follow. To me, "avoid quadratic algorithms" seems like a much more important principle.

[shadowspawn]

For interest the PRs that led to tokens feature and docs are:

• feat: add parsed meta-data to returned properties pkgjs/parseargs#129
• util: add tokens to parseArgs #43459 (comment)

(Over 180 comments, but of course that was for the full feature and not just the example.)

[imcotton]

you should not have a sample where there is a reduce which makes a copy of the whole structure, because that's quadratic.

To me, "avoid quadratic algorithms" seems like a much more important principle.

@bakkot
Out of curiosity, have you run the actual benchmark against two implementations yet?

[imcotton]

Back to Dec 2018, the v8 team had this article: Speeding up spread elements, it's about massive performance improvement of spread on Arrays where require nothing changes from end user, I wander if one day the Object literals got similar treatment, does it shake up any stands on O(n^2)?

I don't believe there is no room for syntax like { foo, ...rest } or { ...rest, bar } to be optimized from internal engine, just like Arrays.

I don't have anything on this PR left, feel free to close as you please.