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 property-no-unknown
false negatives for newer custom syntaxes
#6553
Conversation
🦋 Changeset detectedLatest commit: 54a5098 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 |
property-no-unknown
false negatives for newer custom syntaxes
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.
Thank you, LGTM.
@@ -19,18 +12,6 @@ module.exports = function isStandardSyntaxDeclaration(decl) { | |||
const prop = decl.prop; |
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.
isStandardSyntaxDeclaration
is used in at least 7 rules. Are we sure this is not gonna cause regressions?
Is being more lax already covered by tests?
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.
Unfortunately, tests don't cover. Test using postcss-css-in-js
are skipped:
stylelint/lib/rules/property-no-unknown/__tests__/index.js
Lines 287 to 291 in 5e6d46a
testRule({ | |
skip: true, | |
ruleName, | |
config: [true], | |
customSyntax: 'postcss-css-in-js', |
It seems to have originally existed to detect sources coming from the old css-in-js implementation. Something in the past must have been patching up the node source to contain the Those old syntaxes are deprecated afaik and their newer replacements won't set such a property anymore |
|
I'm not sure, but @Mouvedia's point may be correct. |
[suggest] For compatibility with /**
* @param {unknown} lang
*/
function isStandardSyntaxLang(lang) {
return lang && (lang === 'css' || lang === 'custom-template' || lang === 'template-literal');
}
// ...
module.exports = function isStandardSyntaxDeclaration(decl) {
// ...
// Declarations belong in a declaration block or standard CSS source
if (
parent &&
isRoot(parent) &&
parent.source &&
'lang' in parent.source &&
!isStandardSyntaxLang(parent.source.lang)
) {
return false;
}
// ... |
i feel like you may be misunderstanding. the original PR that introduced it didn't come from nothing, it changed from this: if (parent.type === "root") {
return false;
} to this: if (parent.type === "root" && !isStandardSyntaxLang(parent.source.lang)) {
return false;
} the former would have caused css-in-js syntaxes (old or new ones) to always return false in this statement when they have a top-level declaration (hence the issue you mentioned). the latter just added an escape hatch for those declarations produced by css-in-js syntaxes (such that the before:
after:
by removing it entirely here, we're saying declarations which are a direct child of a root are always standard/valid, not only when produced by css-in-js syntaxes. importantly, we're not going back to the code from before the PR you mentioned. we're getting rid of it entirely. this means all declarations which are children of roots will be valid, including the ones from before. we're expanding the set, not reducing it. that regression/issue shouldn't be affected. |
You are correct.
to filter standard declarations is a pragmatic choice. |
@43081j Thank you for your explanation. Indeed, I misunderstood. 😅
This approach is reasonable. And I believe it will not affect both |
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.
Thank you! LGTM 👍🏼
it seems this is leftover logic from back when we bundled various parts of the css-in-js solutions in the past.
by removing this, newer postcss syntaxes can be correctly used by stylelint (as they often have declarations with no parent rule).
cc @jeddy3