Fix property-no-unknown false negatives for newer custom syntaxes

[43081j]

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

[changeset-bot]

🦋 Changeset detected

Latest commit: 54a5098

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
stylelint	Patch

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

[jeddy3]

Thank you, LGTM.

Ref: 43081j/postcss-styled-components#2 (comment)

[Mouvedia]

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?

[ybiquitous]

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',

[43081j]

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 lang property it was looking for. However, that isn't a property within postcss (if it ever was), the css-in-js code must have been adding it in.

Those old syntaxes are deprecated afaik and their newer replacements won't set such a property anymore

[Mouvedia]

We don't want #3930 to regress, else we will end up in a back and forth.
If there are still code bases where the condition is necessary, it should be an option/flag.
The default would be what you are asking for: lax.

[ybiquitous]

I'm not sure, but @Mouvedia's point may be correct. @stylelint/postcss-css-in-js package depends on postcss-syntax, and the lang property is set in the package. So, problems may come for configs or plugins depending on @stylelint/postcss-css-in-js. See also:

• https://github.com/stylelint/postcss-css-in-js/blob/0.38.0/index.js#L4
• https://github.com/stylelint/postcss-css-in-js/blob/0.38.0/template-parser-helper.js#L209-L211

[ybiquitous]

[suggest] For compatibility with @stylelint/postcss-css-in-js and postcss-syntax, what about changing the logic as below?

/**
 * @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;
	}

	// ...

[43081j]

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 if would be skipped).

before:

• if the declaration is a direct child of a root, it is non-standard

after:

• if the declaration is a direct child of a root and isn't from a css-in-js syntax, it is non-standard

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.

[Mouvedia]

You are correct.
I guess the approach of waiting for issues to be raised if ppl somehow relied on

stylelint/src/utils/isStandardDeclaration.js

Line 10 in 26fa669

if (decl.parent.type === "root") { return false }

to filter standard declarations is a pragmatic choice.

[ybiquitous]

@43081j Thank you for your explanation. Indeed, I misunderstood. 😅

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.

This approach is reasonable. And I believe it will not affect both @stylelint/postcss-css-in-js and postcss-syntax.

[ybiquitous]

Thank you! LGTM 👍🏼