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 "always" option in declaration-empty-line-before #4635

Closed
larrifax opened this issue Mar 4, 2020 · 5 comments
Closed
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

@larrifax
Copy link
Contributor

larrifax commented Mar 4, 2020

Clearly describe the bug

There are false positives for declaration-empty-line-before when adding simple styles using styled-components with stylelints built-in support for css-in-js. See example & demo below for a reproduction.

Might be related to #4003.

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

declaration-empty-line-before

What code is needed to reproduce the bug?

const StyledDiv = styled.div`
  width: 10rem;
`;

What stylelint configuration is needed to reproduce the bug?

{
  "rules": {
    "declaration-empty-line-before": [
      "always",
      {
        "except": [
          "first-nested"
        ]
      }
    ]
  }
}

Which version of stylelint are you using?

13.1.0

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

CLI with stylelint src/**/*.{ts,tsx}. The same issue can be observed on the demo page.

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

Yes, it's related to css-in-js (styled-components).

What did you expect to happen?

No warnings to be flagged.

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

The following errors were flagged:

Expected empty line before declaration (declaration-empty-line-before)
@larrifax
Copy link
Contributor Author

larrifax commented Mar 4, 2020

I just noticed #4574. Maybe this bug should be added to the list?

@jeddy3 jeddy3 changed the title False positives when combining declaration-empty-line-before with css-in-js Fix false positives for "always" option in declaration-empty-line-before Mar 4, 2020
@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 labels Mar 4, 2020
@jeddy3
Copy link
Member

jeddy3 commented Mar 4, 2020

@larrifax Thanks for the clear report, using the template and for creating a reproducible demo.

The issue is either with:

If it's the former, please consider contributing a fix to stylelint itself. There are steps on how to fix a bug in a rule in the Developer guide. If it's the latter, please consider contributing to the parser.

I just noticed #4574. Maybe this bug should be added to the list?

Done.

@larrifax
Copy link
Contributor Author

larrifax commented Mar 5, 2020

I've done a bit of research, and it seems like the problem is that isFirstNested returns false for the above mentioned code. The culprit is the following line:

if (parentNode === undefined || parentNode.type === 'root') {
return false;

In this specific case, parentNode.type === 'root', which results in false being returned. If I remove the root check, the tests I've written passes.
From debugging, I don't see any obvious solution for detecting that this code originates from css-in-js / styled-components, which would allow me to handle this case specially.

From the discussion you had on #4003, I understand that you:

  • don't want to skip the root check entirely, as that would mean the code would pick up on "declarations" outside of rules.
  • don't want to change postcss-jsx to wrap styles in e.g. div on parse, as that would lead to false positives on other rules.

The suggested fix at that time was to introduce some kind of context.isStyled, but it seems that such a solution was never implemented.

@jeddy3 Do you have comments as to how to proceed further? It seems like a decision needs to be made by someone with a more thorough understanding of the codebase and the vision of stylelint than me on this one.

@jeddy3
Copy link
Member

jeddy3 commented Mar 8, 2020

@larrifax Many thanks for digging deeper.

I suggest we change postcss-jsx (now postcss-css-in-js) to wrap styles in a rule-set with an unqualified class selector. I think that's a more accurate representation of what the CSS-in-JS source would look like as vanilla CSS.

and the vision of stylelint than me on this one.

The built-in rules support standard CSS because of the speed of change in the world of CSS-like language extensions and, especially in, CSS-in-JS styling frameworks.

From the docs:

We lean on PostCSS syntaxes to help us with this task [of supporting CSS-like language extensions and CSS-in-JS styling frameworks]. We use them to transform these languages into something that resembles CSS, which is the language that:

  • underpins all the other styling languages
  • is best understood by rules built into stylelint

I think the PostCSS syntaxes should transform the source into an AST that will closely resemble the final CSS. I think that approach would make the built-in rules useful to the most number of people.


We're trying to rally community support for maintaining postcss-css-in-js, so please don't hesitate to get involved in the parser if you have time.

@jeddy3 jeddy3 added the upstream relates to an upstream package label Mar 8, 2020
@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.

@jeddy3 jeddy3 closed this as completed Jan 18, 2022
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

2 participants