tools: always use a trailing comma in arrays

[BridgeAR]

This makes sure all arrays in /lib end with a comma to prevent
future churn. Other options are set as before to minimize the
change.

Something like this just came up in a review and I guess it is indeed a good idea
to slowly migrate to a code base that has trailing commas.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[BridgeAR]

Btw: I personally do not care how this is done but I think we should all agree on one way and enforce that so it does not come up in reviews.

[BridgeAR]

CI https://ci.nodejs.org/job/node-test-pull-request-lite/701/

[vsemozhetbyt]

Refs: #19131, #19133, #5091,

[BridgeAR]

@vsemozhetbyt thanks for pointing these out. @nodejs/collaborators I think we shouldd all agree on one way instead of having such comments in reviews. Therefore I suggest a simple vote and hope we can move forward into that direction afterwards:

🎉 No trailing comma at all
❤️ Always trailing comma

Be aware that moving to always trailing commas would mean that we have to slowly migrate quite a lot of code. The other way around means little code has to be changed.

[Trott]

@BridgeAR I used 👎 for neither suggestion.

I know I once suggested enforcing the comma-dangle rule, but I backed down pretty quickly. The churn was big once you include the entire code base. And more importantly perhaps, I don't think comma comments come up that often. Did one come up recently or something?

If we really want the comma dangle rule, here's my suggestion:

• Open a pull request in the ESLint repo to add a consistent option for comma-dangle such that the rule doesn't care if you always use trailing commas or never use trailing commas, but it does care if you do both of those things in a single file. In other words, be consistent within a file. (Other rules in ESLint have a consistent option that behaves like that.)

• Once that lands and is released, implement that.

[Trott]

Hmmm...it looks like they either no longer had (or never had?) consistent options for whole files, They tend to apply to whole objects, etc. Might be worth opening an issue or chatting in the ESLint gitter first to see if there would be any opposition to a applies-to-a-whole-file consistent option.

[mcollina]

I am -1. We already had to work hard in backporting several PRs due to change of linting. We do not need more as a community.

[tniessen]

Not a fan of dangling commas, ref #19131 (comment)

[Trott]

Hmmm...it looks like they either no longer had (or never had?) consistent options for whole files, They tend to apply to whole objects, etc. Might be worth opening an issue or chatting in the ESLint gitter first to see if there would be any opposition to a applies-to-a-whole-file consistent option.

I asked in the eslint gitter and while a new option would of course need to be discussed, there was no immediate "no, we don't do consistent-within-a-file" so my original suggestion (implement 'consistent' in eslint and then use that in the nodejs code base) is a potentially viable way to go.

[BridgeAR]

I opened an issue in eslint.

I am going to close this now as it is clear that this is not something where we want to go to. Using trailing commas seems like something most people do not want in general as well, so please do not ask for such in reviews.

[bnoordhuis]

The one time I wouldn't object to churn and people vote it down...

Dangling commas reduce the size of the diff when lines are added. It makes code review a bit easier and is helpful with git blame.

I'm of the opinion that once eslint adds an option that lets us migrate on a file-by-file basis, we should.

[mcollina]

I'm definitely 👍 on having a file-by-file rule.