Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jun 13, 2020

Conversation

mdjermanovic
Copy link
Member

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).

@mdjermanovic mdjermanovic added bug ESLint is working incorrectly rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Apr 9, 2020
@aladdin-add
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

mdjermanovic commented Apr 18, 2020

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
Copy link
Member

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
Copy link
Member

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

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
Copy link
Member Author

@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 mdjermanovic changed the title Fix: comma-dangle schema allows multiple options (fixes #13165) Update: disallow multiple options in comma-dangle schema (fixes #13165) Apr 20, 2020
@mdjermanovic
Copy link
Member Author

@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
Copy link
Member

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

@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jun 12, 2020
@mdjermanovic
Copy link
Member Author

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
Copy link
Member

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

@kaicataldo kaicataldo merged commit cbd0d00 into master Jun 13, 2020
@kaicataldo kaicataldo deleted the issue13165 branch June 13, 2020 00:13
@kaicataldo
Copy link
Member

Thanks for contributing!

@ljharb
Copy link
Sponsor Contributor

ljharb commented Aug 2, 2020

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
Copy link
Sponsor Contributor

ljharb commented Aug 2, 2020

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
Copy link
Member Author

[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
Copy link
Sponsor Contributor

ljharb commented Aug 2, 2020

@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
Copy link
Member Author

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
Copy link
Sponsor Contributor

ljharb commented Aug 2, 2020

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".

ljharb added a commit to ljharb/list-exports that referenced this pull request Aug 2, 2020
 - eslint TODO due to eslint/eslint#13166 (comment), breaking change in eslint v7.3
@mdjermanovic
Copy link
Member Author

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

@btmills
Copy link
Member

btmills commented Aug 13, 2020

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
Copy link
Sponsor Contributor

ljharb commented Aug 13, 2020

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

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Dec 11, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comma dangle syntax no longer ignores functions: never
6 participants