-
-
Notifications
You must be signed in to change notification settings - Fork 47
Use postcss instead of @adobe/css-tools for css parsing #314
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
Conversation
jgerigmeyer
commented
Jun 30, 2025
- Replaces @adobe/css-tools with postcss for CSS parsing and printing
- Adds prettier for consistent formatting of printed CSS
- Fixes sass-true fails to parse non-standard at-rules #313
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read through the code, and the changes look good. A few questions, but nothing blocking!
const parseAssertion: Parser = function (rule, ctx) { | ||
if (isCommentNode(rule)) { | ||
const text = rule.comment?.trimStart(); | ||
const text = rule.text.trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is changing from trimStart()
to trim()
. Would this account for the difference in https://github.com/oddbird/true/pull/314/files#r2176052308?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No -- PostCSS automatically trims the whitespace from comments, and stores the original whitespace in rule.raws.left
and rule.raws.right
. So I think this trim()
isn't really needed, but it also doesn't hurt.
passed: true, | ||
output: | ||
'/* Some loud comment */\n\n.test-output {\n -property: value;\n}', | ||
'/* Some loud comment */\n.test-output {\n -property: value;\n}', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a breaking change? Or just an internal change that won't impact users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is internal, and shouldn't impact users.
@mirisuzanne Does this look okay to you? If so, I might do an alpha release so that users can test it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jgerigmeyer I don't know most of the internals here, but if the tests pass, it looks fine to me. :)