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

Some errors not reported #2

Open
karlhorky opened this issue Dec 30, 2022 · 3 comments
Open

Some errors not reported #2

karlhorky opened this issue Dec 30, 2022 · 3 comments

Comments

@karlhorky
Copy link

karlhorky commented Dec 30, 2022

Hi @43081j 👋 First of all, thanks for all of your hard work on this and also postcss-js-core, so amazing!

It appears that some Stylelint errors are not reported with customSyntax: 'postcss-styled-components' along with setting some standard rules using extends: ['stylelint-config-recommended'] (eg. such as property-no-unknown):

import styled, { css } from 'styled-components';

const Div = styled.div`
  /* ✅ declaration-block-no-duplicate-properties reported */
  color: red;
  color: red;
  /* ❌ property-no-unknown not reported */
  booooorder: 1px solid red;
`;

const divStyles = css`
  /* ❌ declaration-block-no-duplicate-properties not reported */
  color: red;
  color: red;
  /* ❌ property-no-unknown not reported */
  booooorder: 1px solid red;
`;

It seems like the css tagged template literals are possibly not checked at all? 🤔

This is also similarly broken when using Emotion:

import { css } from '@emotion/react';
import styled from '@emotion/styled';

const Div = styled.div`
  /* ✅ declaration-block-no-duplicate-properties reported */
  color: red;
  color: red;
  /* ❌ property-no-unknown not reported */
  booooorder: 1px solid red;
`;

const divStyles = css`
  /* ❌ declaration-block-no-duplicate-properties not reported */
  color: red;
  color: red;
  /* ❌ property-no-unknown not reported */
  booooorder: 1px solid red;
`;

I've created an interactive demo of this on Replit.

To use it, change to the Shell tab on the right and run npm install and then npm run lint.

Demo (Replit): https://replit.com/@karlhorky/customSyntax-postcss-styled-components#styled-components.js

Screenshot 2022-12-30 at 23 17 36


Note that this does not use the nesting or other features of Sass (eg. to test out #1) because I haven't been able to get customSyntax: "postcss-styled-components/scss" working yet, as I mentioned here:

I will open new issues about any problems with these features once I can get the other entrypoints working.

@43081j
Copy link
Owner

43081j commented Jan 2, 2023

@jeddy3 the property-no-unknown rule fails against the new syntaxes I published because of this it seems:

https://github.com/stylelint/stylelint/blob/797381ced3190aa0c78bff8fb892572caadf4305/lib/utils/isStandardSyntaxDeclaration.js#L23-L32

do you have any idea where lang came from? (as in source.lang) i can't find it in the postcss types and it seems stylelint has to cast it too in the JSDoc... does that mean its a stylelint-specific thing?

just trying to figure out the fix here. seems to be a choice between:

  • add lang: 'css' to these custom syntaxes
  • loosen the lang constraint in stylelint

but im guessing there's a good reason for it so it should probably be the former rather than the latter, once i understand it

FYI the styled components syntax in particular has a questionable AST structure in that it has a Root { Decl, Decl, ... } (i.e. there is no Rule owning the declarations). This is because in styled-components, you don't really ever define whole rules but rather the contents of them.

@jeddy3
Copy link

jeddy3 commented Jan 2, 2023

do you have any idea where lang came from?

I think it's left over from when we used to include postcss-syntax in Stylelint. It was hacky stuff, and I think we can remove this conditional in our upcoming 15.0.0 release. (Our tests pass when I removed it...). You're welcome to open a pull request (off the v15 branch) to do this and test against that.

FYI the styled components syntax in particular has a questionable AST structure in that it has a Root { Decl, Decl, ... }

I'm not too familiar with custom syntaxes, but I think this will be okay as I assume the postcss-html syntax does the same for inline styles, e.g.:

<a style="color: red; padding: 1rem;">foo</a>

And we'll want to treat those in Stylelint as valid declarations (even if they don't belong to a rule). If we don't already, then it's a bug in our isStandard* utils that we can fix.

@43081j
Copy link
Owner

43081j commented Jan 2, 2023

I think it's left over from when we used to include postcss-syntax in Stylelint. It was hacky stuff, and I think we can remove this conditional in our upcoming 15.0.0 release. (Our tests pass when I removed it...). You're welcome to open a pull request (off the v15 branch) to do this and test against that.

great, i was hoping that's the case :D ill open a PR if you haven't already

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants