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 false positives for JS variables in value-keyword-case #5185

Closed
Tracked by #4574
jsg2021 opened this issue Mar 5, 2021 · 10 comments
Closed
Tracked by #4574

Fix false positives for JS variables in value-keyword-case #5185

jsg2021 opened this issue Mar 5, 2021 · 10 comments
Labels
status: ready to implement is ready to be worked on by someone syntax: css-in-js relates to JS objects & template literals type: bug a problem with a feature or rule upstream relates to an upstream package

Comments

@jsg2021
Copy link

jsg2021 commented Mar 5, 2021

Clearly describe the bug

stylelint is checking and flagging/fixing javascript as if it were css.

Which rule, if any, is the bug related to?

all? value-keyword-case in this example.

What code is needed to reproduce the bug?

some file named with .jsx extension

demo.jsx:

function Foo(props) {
	const myBackground = '....';
	const style = {
		...props.style,

		backgroundImage: myBackground ? myBackground : 'url(...)',
	};

	return <div {...props} style={style} />;
}

What stylelint configuration is needed to reproduce the bug?

e.g.

{
  "extends": [
    "stylelint-config-standard"
  ]
}

Which version of stylelint are you using?

13.11.0

How are you running stylelint: CLI, PostCSS plugin, Node.js API?

VS code extension and cli... my cli ignores JS files for now. But when I save in vs code, it breaks if I allow its save-action.

Does the bug relate to non-standard syntax (e.g. SCSS, Less etc.)?

no

What did you expect to happen?

No validation/fixing on js. CSS in tag template literals is fine to check because that IS css. But JS properties should not be validated.

What actually happened (e.g. what warnings or errors did you get)?

CSS errors are flagged. (and fixes attempted)

demo.jsx
 4:2   ✖  Expected indentation of 4 spaces               indentation       
 6:2   ✖  Expected indentation of 4 spaces               indentation       
 6:19  ✖  Expected "myBackground" to be "mybackground"   value-keyword-case
 6:34  ✖  Expected "myBackground" to be "mybackground"   value-keyword-case
 7:2   ✖  Expected indentation of 2 spaces               indentation
@jeddy3 jeddy3 added status: ready to implement is ready to be worked on by someone syntax: css-in-js relates to JS objects & template literals type: bug a problem with a feature or rule upstream relates to an upstream package labels Mar 8, 2021
@jeddy3 jeddy3 changed the title False Positive for JS code Fix false positives for JS variables in value-keyword-case Mar 8, 2021
@jeddy3 jeddy3 mentioned this issue Mar 8, 2021
23 tasks
@jeddy3
Copy link
Member

jeddy3 commented Mar 8, 2021

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

I can reproduce this using the demo.

It's likely a problem with the parser stylelint uses to support CSS-in-JS. I've added this issue to #4574, which is a growing list of the problems with the syntax that we need help to fix.

Please consider contributing to the parser if you have time.


VS code extension and cli... my cli ignores JS files for now. But when I save in vs code, it breaks if I allow its save-action.

You can either:


No validation/fixing on js. CSS in tag template literals is fine to check because that IS css. But JS properties should not be validated.

The majority of the built-in rules are applicable to CSS-in-JS object notation, especially the possible errors and feature limiting rules. Even a number of stylistic rules are applicable to strings within object notation, e.g. the following:

const Button = styled("button", {
  width: "Min( 100VW ,  .1%)"
}

Will be fixed to:

const Button = styled("button", {
  width: "min(100vw,  0.1%)"
}

@jsg2021
Copy link
Author

jsg2021 commented Mar 8, 2021

Thanks!

No validation/fixing on js. CSS in tag template literals is fine to check because that IS css. But JS properties should not be validated.

The majority of the built-in rules are applicable to CSS-in-JS object notation, especially the possible errors and feature limiting rules. Even a number of stylistic rules are applicable to strings within object notation, e.g. the following:

const Button = styled("button", {
  width: "Min( 100VW ,  .1%)"
}

Will be fixed to:

const Button = styled("button", {
  width: "min(100vw,  0.1%)"
}

ah, that's a good point. My statement was too broad. I was specifically thinking about identifiers, not string literals.

@jsg2021
Copy link
Author

jsg2021 commented Mar 8, 2021

I've added this to my code config to prevent js/css lint conflicting fix loops:

"editor.codeActionsOnSave": {
    "source.fixAll": true,
    "source.fixAll.stylelint": false
  },

@jsg2021
Copy link
Author

jsg2021 commented Mar 8, 2021

Did some digging.

const parsed = valueParser(getDeclarationValue(decl));

At this point, the value in the example is a JS expression, and the decl.parent.type is object... parsing the js expression as a CSS value. I would guess that it should ignore non-string-literal values in the expression?

@jeddy3
Copy link
Member

jeddy3 commented Mar 9, 2021

Thanks for digging.

I would guess that it should ignore non-string-literal values in the expression?

The CSS-in-JS syntax should probably substitute JS expressions for dummy values in the PostCSS AST.

For example:

backgroundImage: myBackground ? myBackground : 'url(...)',

Becomes:

{
  "type": "root",
  "nodes": [
    {
      "type": "decl",
      "prop": "background-image",
      "value": "$dummyvalue"
    }
  ],
}

There's experimental work looking into doing this for interpolation in template literals.

@ahuglajbclajep
Copy link

There is a same problem with function-name-case.
stylelint-disable comments do not work for these.

@Alecell
Copy link

Alecell commented Apr 8, 2021

Same here, I've been trying to ignore the ${} pattern but without success.

There isn't a work around on this?

Edit:
I barely manage to make a work around on this, in my case I could only ignore all patterns that have a . and _ on the value, so something like foo.barBaz or FOO_BAR_BAZ will be ignored, but I'm certain that I'll need to update that pattern very soon.

Thats the pattern if someone is interested: "ignoreKeywords": ["/(\\.|_)+/"]

@rbong
Copy link

rbong commented Jun 5, 2021

The CSS-in-JS syntax should probably substitute JS expressions for dummy values in the PostCSS AST.

Is that the best approach? It just seems like postcss-value-parser isn't meant to parse CSS-in-JS values. Maybe a new parser could help traverse CSS-in-JS values more accurately? I know that it would be a lot of work to develop a new parser and update rules, but all of the information you need to distinguish Javascript variables from strings is in the node, it's only discarded when the declaration value is read as a plain string and parsed.

@kachkaev
Copy link

kachkaev commented Jul 8, 2021

I think I’m having the same issue in JSX:

const MyComponent = () => {
  const minWidth = 42;
  return <div style={{ minWidth }}>hello</div>;
}

This shows: Expected "minWidth" to be "minwidth" (value-keyword-case) stylelint(value-keyword-case).

We already have .stylelintignore with the following contents:

## file extensions
*.*
LICENSE
!*.css
!*.less


## shared between linters
...


## copied from .gitignore
...

(this approach to .stylelintignore file structure is described in prettier/prettier#6280 (comment))

All works in CLI, but because of a bug in the VSCode extension, the false positive still shows up in the editor. It is possible to set "stylelint.validate": ["css", "less"] in VSCode, but that only works for user preferences, so cannot be shared with the team via [project]/.vscode/settings.json.

Interestingly, replacing style={{ minWidth }} with blah={{ minWidth }} removes the error. Is there any workaround that would help ignore value-keyword-case by saying ‘don’t look at jsx → style prop’? 🤔

@jeddy3
Copy link
Member

jeddy3 commented Jan 18, 2022

Closing as stylistic rules are frozen. The community is welcome to migrate the rule to a plugin and fix this bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready to implement is ready to be worked on by someone syntax: css-in-js relates to JS objects & template literals type: bug a problem with a feature or rule upstream relates to an upstream package
Development

No branches or pull requests

6 participants