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

Potential support for double-slash comments. #11

Open
tanner-ropp opened this issue Mar 27, 2023 · 3 comments
Open

Potential support for double-slash comments. #11

tanner-ropp opened this issue Mar 27, 2023 · 3 comments

Comments

@tanner-ropp
Copy link

Hello! I see in the readme that there is hesitancy to adding support for double-slash comments within styled components, but with it being a syntax error rather than a rule, it can create some roadblocks in the developer experience. Because they aren't parsed properly, the developer doesn't have an explicit explanation of what caused the error (for example the no double slash comments rule). In larger organizations where it might not be widespread knowledge that we should not be using double-slash comments in styled components, there is unclear messaging on how to move past this issue. As a workaround currently (in my own fork), I'm modifying the tokenizer to classify double slash comments the same as block comments so developers don't encounter this. Is this, or a similar solution where we can have the double-slash moments parsed normally, something you would consider merging? I'd be happy to write a PR. Thanks!

@tanner-ropp
Copy link
Author

I find this messaging:
Screen Shot 2023-03-27 at 1 36 38 PM

to be more explicit and helpful than this messaging:
Screen Shot 2023-03-27 at 1 36 29 PM

let me know what you think! This package is really helpful and I would love to be able to use it in our project. Thanks again

@hudochenkov
Copy link
Owner

Hello! I see in the readme that there is hesitancy to adding support for double-slash comments within styled components, but with it being a syntax error rather than a rule, it can create some roadblocks in the developer experience.

Hesitance comes from few points:

  1. I don't want to spend time implementing and maintaining it.
  2. For a 4 years previous CSS-in-JS syntax for PostCSS existed I don't remember much complain about it not supporting double-slash comments.
  3. Standard CSS comments work great. While it is possible to use double-slash comments in Styled Components, I see no point of encouraging it.

Because they aren't parsed properly, the developer doesn't have an explicit explanation of what caused the error (for example the no double slash comments rule). In larger organizations where it might not be widespread knowledge that we should not be using double-slash comments in styled components, there is unclear messaging on how to move past this issue.

I agree messaging is not clear. It is comming directly from PostCSS default parser. I tried to change as minimal as possible in the parser. Because I'm not a parser expert, and I would rather rely on PostCSS authors experience. Even for future updates of a parser.

Stylelint with default parser also shows “Unknown word” error. no-invalid-double-slash-comments could detect only few cases, and only if parser could parse styles.

Is this, or a similar solution where we can have the double-slash moments parsed normally, something you would consider merging? I'd be happy to write a PR. Thanks!

I'm not sure it worth the effort to complicate parser with double-slash comments. As history showed us, it is not a big deal that they are not supported.

Also, adding support for it will make future update to parser harder. As the idea is to backport changes from PostCSS default parser back to this parser as CSS spec evolves.

@mctrafik
Copy link

-1 for this feature request. We are so happy this is a default rule in the checker because all minifiers strip out whitespace and if there are any // in the CSS the remainder of it gets considered a comment.

Obvious example:

body {
  color: red;
}
// override body color
body {
  color: blue;
}

gets minified to:

body{color:red;}// override body color body{color:blue;}

and as you can imagine doesn't have the desired effect.

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