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

Deprecate resolveNestedSelectors option of selector-class-pattern #7482

Open
ybiquitous opened this issue Jan 21, 2024 · 9 comments
Open

Deprecate resolveNestedSelectors option of selector-class-pattern #7482

ybiquitous opened this issue Jan 21, 2024 · 9 comments
Labels
status: needs discussion triage needs further discussion

Comments

@ybiquitous
Copy link
Member

ybiquitous commented Jan 21, 2024

What is the problem you're trying to solve?

The resolveNestedSelectors secondary option of the selector-class-pattern rule can resolve Sass-like identifier concatenation (e.g. &__foo), but this behavior is against the current rule policy of "standard CSS syntax only":

A rule must be:
- for standard CSS syntax only

The CSS Nesting spec forbids such a syntax explicitly:

Unfortunately, this string-based interpretation is ambiguous with the author attempting to add a type selector in the nested rule.

What solution would you like to see?

I think 3rd-party plugins like stylelint-scss should provide this option instead. So, I suggest deprecating the option and finally removing it in the next major version.

For example:

  1. Depreacate the option with a warning in the next minor version (e.g. 16.3.0)
  2. Remove the option in the next major version (e.g. 17.0.0)

Currently, selector-class-pattern supports the standard nesting with resolveNestedSelectors: false (default). See the demo. So, I think most users would not have trouble without this option.

In addition, from the point of view of a maintainer, the following code in the rule would be no longer necessary and be able to be much simpler, including a dependency on postcss-resolve-nested-selector:

if (shouldResolveNestedSelectors && hasInterpolatingAmpersand(selector)) {
for (const nestedSelector of resolveNestedSelector(selector, ruleNode)) {
if (!isStandardSyntaxSelector(nestedSelector)) {
continue;
}
parseSelector(nestedSelector, result, ruleNode, (s) => checkSelector(s, ruleNode));
}
} else {

See also

@ybiquitous ybiquitous added the status: needs discussion triage needs further discussion label Jan 21, 2024
@Mouvedia
Copy link
Contributor

I think an exploratory review of all the rules is a preliminary to the deprecation.
i.e. all rules that have similar incompatibilities with CSS nesting should be handled at once

@kristerkari what's your opinion on

I think 3rd-party plugins like stylelint-scss should provide this option instead.

?

@ybiquitous
Copy link
Member Author

I think an exploratory review of all the rules is a preliminary to the deprecation.

Good point. Agree with this idea.

@ybiquitous
Copy link
Member Author

ybiquitous commented Jan 21, 2024

When I walk through all the rules quickly, the following rules seem to be related to non-standard syntaxes like SCSS:

Affected by a rule itself:

see #7482 (comment)

Affected by a rule option:

Affected by an undocumented behavior of postcss-resolve-nested-selector:

@ybiquitous
Copy link
Member Author

Extending such rules in 3rd-parties (e.g. stylelint-scss) might be very tough... 😓

@kristerkari
Copy link
Contributor

Thanks for the ping 👍

I don't mind moving things to stylelint-scss, but I'm also not so keen on just copying an existing rule from stylelint and re-implementing it in stylelint-scss with some small difference, since that means that the rule needs to be then maintained in two places.

@ybiquitous
Copy link
Member Author

@kristerkari Your concern makes sense. I can't get a nice idea for now. 😓

Probably, we should have supported the resolveNestedSelectors option for all the rules depending on postcss-resolve-nested-selector's identifier concatenation.

@jeddy3
Copy link
Member

jeddy3 commented Jan 21, 2024

We typically provide secondary options, like ignore*: [], to help plugin authors reuse code via the checkAgainstRule util, e.g. scss/at-rule-no-unknown. Would it be possible to add, to these rules, a secondary option that accepts a function to transform the selectors?

We can change these rules to use a spec-based transformer, i.e. one that desugars to :is() and doesn't support string concatenation, and plugin authors can reuse the rules by providing a transformer, e.g. for SCSS that'll be postcss-resolve-nested-selector.

Affected by a rule itself: no-invalid-double-slash-comments

This rule is for a hack/trick in standard CSS where you can use double-slashes to put an invalid value into the stylesheet, effectively creating an "ignore next construct" comment. It's a dodgy pattern, so we have a rule to disallow it.

@ybiquitous
Copy link
Member Author

Would it be possible to add, to these rules, a secondary option that accepts a function to transform the selectors?

Adding an ignore* secondary option sounds great! If this approach works, we can switch from postcss-resolve-nested-selector to @csstools/selector-resolve-nested, which is a spec-compliant package authored by @romainmenke yesterday.

This rule is for a hack/trick in standard CSS where you can use double-slashes to put an invalid value into the stylesheet, effectively creating an "ignore next construct" comment. It's a dodgy pattern, so we have a rule to disallow it.

Ah, I see it. I didn't read the rule README carefully. 😓

@jeddy3
Copy link
Member

jeddy3 commented Jan 28, 2024

We can put together a PoC PR to test the transformer secondary option theory (branching off #7496). If it's viable, then we have a way forward that'll let us modernise our support spec compliance and also help plugin authors reuse the rules.

@Mouvedia Mouvedia mentioned this issue Mar 17, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs discussion triage needs further discussion
Development

No branches or pull requests

4 participants