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 function-calc-no-unspaced-operator
false negatives
#7655
Conversation
🦋 Changeset detectedLatest commit: c8d7979 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
b8713a9
to
417458c
Compare
This significantly changes the implementation by switching `postcss-value-parser` to `@csstools/css-parser-algorithms`. Because `postcss-value-parser` is inactive and leaves bugs for a while, and we've used already `@csstools/css-parser-algorithms` for some rules. In addition, this updates `tsconfig.json` to support ES2023 libraries. This change uses `Array.prototype.findLast()`, which has been supported since Node.js 18.0.0. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/findLast
417458c
to
b673910
Compare
@romainmenke If I have done something wrong with |
🎉 |
quick heads up, I will review this by Monday at the latest :) |
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 looks really good @ybiquitous, thank you for this!
I've left a few notes here and also took a deep dive into this rule to see how I would approach it : #7670
There aren't any major issues with this change as far as I can tell 🎉
Can you update this line? |
endColumn: 24, | ||
}, | ||
{ | ||
code: 'a { padding: calc(1px+2px+3px); }', |
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 one should be 'a { padding: calc(1px+2px + 3px); }'
to catch #7181 regression.
i.e. invalid followed by valid
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.
But, autofixing to calc(1px + 2px + 3px)
is better than calc(1px+2px + 3px)
, isn't 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.
Look at the demo linked in the issue; as you can see it's not detected as invalid right now but calc(1px+2px+3px)
is.
Hence we want calc(1px+2px + 3px)
way more because it is/was a bug.
i.e. we want to pick an eventual regression
minor
non-blocking
lib/rules/function-calc-no-unspaced-operator/__tests__/index.mjs
Outdated
Show resolved
Hide resolved
Thanks @romainmenke, having such a proactive member adapting our dependencies that fast is invaluable. |
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.
LGTM!
Thank you @ybiquitous for this update 🙇
Thanks for the review, all! |
Closes #7181
Closes #7618
This also fixes a bug reported on #7646 (comment).
This significantly changes the implementation by switching
postcss-value-parser
to@csstools/css-parser-algorithms
. Becausepostcss-value-parser
is inactive and leaves bugs for a while, and we've used already@csstools/css-parser-algorithms
for some rules.In addition, this updates
tsconfig.json
to support ES2023 libraries. This change usesArray.prototype.findLast()
, which has been supported since Node.js 18.0.0.