Update: disallow multiple options in comma-dangle schema (fixes #13165)

[mdjermanovic]

Prerequisites checklist

• I have read the contributing guidelines.
• The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

What is the purpose of this pull request? (put an "X" next to an item)

[X] Bug fix

fixes #13165

comma-dangle schema mistakenly allows specifying multiple options.

User might expect that this rule allows string option + specific overrides in an additional object option, because some other rules indeed have such configuration.

What changes did you make? (Give an overview)

Added additionalItems: false in order to allow specifying only 1 option.

Is there anything you'd like reviewers to focus on?

This change can break a build with an invalid configuration, which should be a good thing since it wasn't working as expected.

A similar change was treated as a semver-patch bug fix (#12639).

[aladdin-add]

This change can break a build with an invalid configuration, which should be a good thing since it wasn't working as expected.

I slightly suggest it to be a breaking change, but will follow the team's consensus.

plus, I noticed quite a few rules didn't have the option(e.g.arrow-body-style), do we want to "fix" the issue in all other rules?

[mdjermanovic]

plus, I noticed quite a few rules didn't have the option(e.g.arrow-body-style), do we want to "fix" the issue in all other rules?

I think we should fix this if there is a similar problem in other rules, but arrow-body-style seems to be working fine?

Some rules have maxItems property set. While I believe additionalItems is more appropriate for tuple validation (most of the rules), maxItems also works well.

[anikethsaha]

I think it would be good to add these check-in internal-rule to go through to the schema and if key is object then it should have additionalProperties set as false.

WDYT ?

[mdjermanovic]

I think it would be good to add these check-in internal-rule to go through to the schema and if key is object then it should have additionalProperties set as false.

It might be interesting to check this and maybe open a separate issue.

I'd expect that the vast majority of "object" configs in core rules should indeed have additionalProperties: false, but maybe not all.

A simple search by type: "object" gives 180 results, while a search by additionalProperties: false gives 169 results.

4 objects have something different as additionalProperties and it seems to be a correct config for those rules, e.g., space-unary-ops config for "overrides".

The absence of additionalProperties key in the remaining 7 objects might be a bug.

[anikethsaha]

I'd expect that the vast majority of "object" configs in core rules should indeed have additionalProperties: false, but maybe not all.

yes,. about those rules not having this set, I think those object types having no properties key doesn't have to be additionalProperties: false set, I saw in some rule, forgot which one.

[anikethsaha]

@mdjermanovic I have created a bit of rule to check for the additionalProperties

• implementation demo 1
• implementation demo 2
• implementation demo 3

implementing the same code for no-invalid-meta didn't properly, not sure why (maybe I am using babel-eslint in ASTExplorer so it may differ)

I think I might have missed some cases.
also, this code is a bit non-robust code as the complexity is not so good cause of those nest loops.
so if you think these kinds of checks may be helpful, I would love to get some better approach than this.

[mdjermanovic]

@anikethsaha I found that rules that don't have additionalProperties key for type: "object" indeed should have that key (see #13198 and #13199), so it might be good to open an issue to evaluate this check as an internal rule.

[mdjermanovic]

@aladdin-add it's now changed to Update.

I'm not sure if this should be treated as a breaking change, but it indeed seems like a change that requires a semver-minor release rather than a patch.

[aladdin-add]

there was a similar case: 2d32a9e -- it was treated as a breaking change.

[mdjermanovic]

I marked this as accepted since I believe this is a bug that should be fixed, but there's still open question on how to treat this: semver-minor or breaking.

[kaicataldo]

I think this should be treated as a semver-minor bug fix.

[kaicataldo]

Thanks for contributing!

[ljharb]

Heads up: this was a breaking change in eslint v7.3. comma-dangle used to accept [severity, string default, object of overrides], and since 7.3, now only accepts [severity, string] or [severity, object of overrides].

This should be reverted, or fixed ASAP so the object can be used in conjunction with the string.

[ljharb]

Specifically, I'm confused why the choice was made to break users who have a perfectly valid and precedented expectation (that the string sets the default, and the object provides granular overrides for it) rather than fixing the rule to match that expectation.

[mdjermanovic]

[severity, string default, object of overrides] wasn't documented, was never intended to work, and never actually worked as it would be expected, so I'm of an opinion that it is correct to disallow such configuration at some point.

I think we should have a discussion about whether or not this kind of changes requires a major version, and maybe update the semver policy with this detail.

[ljharb]

@mdjermanovic being undocumented isn’t license to break it in a nonmajor.

Either way, this is breaking a few of my packages that were previously working fine with this setting (there’s other rules that work this way), so I’d appreciate a revert or a proper fix so it does work this way :-)

[mdjermanovic]

Either way, this is breaking a few of my packages that were previously working fine with this setting (there’s other rules that work this way), so I’d appreciate a revert or a proper fix so it does work this way :-)

But the revert wouldn't do any good for those packages, you would still have to change the config since it doesn't work as it was expected when the config was made?

[ljharb]

That doesn't matter - this PR caused the CI job for linting to fail where it previously succeeded. Additionally, in the case I'm worried about, it's actually always worked correctly, because my string is the same value as "every RHS of the object".

[mdjermanovic]

Added an item for discussion eslint/tsc-meetings#198 (comment)

[btmills]

To keep you up to date @ljharb, after today's TSC meeting, we're updating our semver policy to include rule schemas as part of the public API, for which incompatible changes are semver-major. While we don't currently have the infrastructure to patch the previous minor version where this was first released, we're reverting this change as part of this weekend's release.

[ljharb]

@btmills thank you all for the decision, the followup, and the revert.