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

Fix invalid option warning for string properties in *-list #5933

Closed
edg2s opened this issue Feb 22, 2022 · 9 comments · Fixed by #5934
Closed

Fix invalid option warning for string properties in *-list #5933

edg2s opened this issue Feb 22, 2022 · 9 comments · Fixed by #5934
Labels
status: wip is being worked on by someone type: bug a problem with a feature or rule

Comments

@edg2s
Copy link

edg2s commented Feb 22, 2022

What steps are needed to reproduce the bug?

The config below fails in 14.5.2 (but worked in 14.5.1):

Invalid Option: Invalid option value "{"/^border/":"none","/^outline/":"none"}" for rule "declaration-property-value-disallowed-list"

What Stylelint configuration is needed to reproduce the bug?

{
  "rules": {
    "declaration-property-value-disallowed-list": {
      "/^border/": "none",
      "/^outline/": "none"
    }
  }
}

How did you run Stylelint?

CLI

Which version of Stylelint are you using?

14.5.2

What did you expect to happen?

No Invalid Option warning

What actually happened?

Invalid Option warning

Does the bug relate to non-standard syntax?

No response

Proposal to fix the bug

No response

@edg2s
Copy link
Author

edg2s commented Feb 22, 2022

Might have been caused by #5924?

@jeddy3 jeddy3 changed the title declaration-property-value-disallowed-list not working in 14.5.2 Fix invalid option for warning for string properties in declaration-property-value-disallowed-list Feb 22, 2022
@jeddy3
Copy link
Member

jeddy3 commented Feb 22, 2022

@edg2s Thanks for the report and for using the template.

The rule expects an array:

{
  "rules": {
    "declaration-property-value-disallowed-list": {
      "/^border/": ["none"],
      "/^outline/": ["none"]
    }
  }
}

But it seems we had undocumented behaviour of accepting strings.

What's the appropriate thing to do in this situation? If people are relying on this behaviour, should we make the options validator more lenient?

@jeddy3 jeddy3 added status: needs discussion triage needs further discussion status: ready to implement is ready to be worked on by someone type: bug a problem with a feature or rule and removed status: needs discussion triage needs further discussion labels Feb 22, 2022
@jeddy3
Copy link
Member

jeddy3 commented Feb 22, 2022

We allow strings for other *-(dis)allowed-list rules, for example.

To be consistent, we should officially allow strings in this rule too. (It was a historical design decision mistake to allow strings in the other rules as it needlessly complicates things, but we are where we are now.)

We'll need to loosen up validateObjectWithArrayProps util.

I've labelled the issue as ready to implement. Please consider contributing if you have time.

@edg2s
Copy link
Author

edg2s commented Feb 22, 2022

But it seems we had undocumented behaviour of accepting strings.

Sounds like a grey area, but it would be best to avoid breaking changes (even for undocumented features) in minor releases.

Right now we are in position where will have to release a new version of our shared config to prevent users seeing errors while doing normal minor version updates.

@ybiquitous
Copy link
Member

Thanks for the report. I think validateObjectWithArrayProps may be unnecessary. The following change allows both patterns: "prop": "string" or "prop": ["array", "of", "string"]

--- a/lib/rules/declaration-property-value-disallowed-list/index.js
+++ b/lib/rules/declaration-property-value-disallowed-list/index.js
@@ -25,7 +25,7 @@ const rule = (primary) => {
 	return (root, result) => {
 		const validOptions = validateOptions(result, ruleName, {
 			actual: primary,
-			possible: [validateObjectWithArrayProps([isString, isRegExp])],
+			possible: [isString, isRegExp],
 		});
 
 		if (!validOptions) {

@ybiquitous
Copy link
Member

Oh, sorry, I misunderstood. The change above (#5933 (comment)) doesn't work. 😓

@ybiquitous
Copy link
Member

I've opened PR #5934 to fix the problem (still draft).

@jeddy3 jeddy3 added status: wip is being worked on by someone and removed status: ready to implement is ready to be worked on by someone labels Feb 23, 2022
@jeddy3 jeddy3 changed the title Fix invalid option for warning for string properties in declaration-property-value-disallowed-list Fix invalid option for warning for string properties in *-list Feb 23, 2022
@jeddy3 jeddy3 changed the title Fix invalid option for warning for string properties in *-list Fix invalid option warning for string properties in *-list Feb 23, 2022
@jeddy3
Copy link
Member

jeddy3 commented Feb 23, 2022

@edg2s Thanks again for reporting the issue.

Fix released in 14.5.3.

@edg2s
Copy link
Author

edg2s commented Feb 23, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: wip is being worked on by someone type: bug a problem with a feature or rule
Development

Successfully merging a pull request may close this issue.

3 participants