-
-
Notifications
You must be signed in to change notification settings - Fork 942
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
Ignore ERB template tags in selectors #4491
Conversation
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.
Good job!
@stylelint/core What we decided about release? (second part of my top message) |
Sounds good for me |
|
@hudochenkov thanks for the quick fix! stylelint/lib/rules/no-descending-specificity/index.js Lines 79 to 81 in 42f9bf2
As I can reproduce the same problem with no-descending-specificity rule
As far as I understand this condition should be checked before trying to parse selector. |
@vilchik-elena make sense! Can you send a PR, please? |
@hudochenkov I tried to do that, but I fail to reproduce with unit test :( (while I do with CLI with the same Also I checked that that ~40 usages of |
@vilchik-elena I believe this test will be ok: testRule(rule, {
ruleName,
config: [true],
syntax: 'html',
skipBasicChecks: true,
accept: [
{
code: `<style>
.highlight {
padding: 2px;
}
<%- SEVERITY_COLORS.each do |severity, color| %>
.severity.<%= severity %> {
color: <%= color %>;
}
.highlight.<%= severity %> {
background-color: <%= color.fade_out(0.4) %>;
}
</style>`,
description: 'ERB templates are ignored',
},
],
});
Let's fix rules which confirmed has issues. Maybe we can change |
Ok, I finally found out what's the problem. I was trying to create patch from master branch. And the problem of So what should be done next according to you @hudochenkov ? |
Good catch :) In commit you found we added stylelint/lib/rules/no-descending-specificity/index.js Lines 53 to 56 in 42f9bf2
|
@hudochenkov so when this will be part of release? |
We plan to release in the beginning of January. |
Fixes #4489.
Problem lies in many places. We are passing selector
<% COLORS.each do |color| %>\na
topostcss-selector-parser
, and the later falls into endless loop. Which is understandable. This case might be fixed inpostcss-selector-parser
, but it won't happen any time soon. Currently, we're usingpostcss-selector-parser@3.1.0
, while the latest version is6.0.2
. We can't upgrade, because there are still some issues in new version of a parser which needs to be resolved in a parser.So, the only choice we have is to fix problem in stylelint. My solution is not universal, but if will fix the problem. It might create false negatives for
isStandardSyntaxSelector
, when selector like[data-something="<%"]
is used. I think it's very unlikely to have a selector with<%
or%>
in it.There is also a question when to release the fix. The next version of stylelint should be major, because we have some breaking changes in
master
. On the other hand we want to drop Node.js 8 from January 1. Which will require to release new major version. I don't want to release two major versions, as it's bad UX for our users have to update two major versions.If this fix has to be release as soon as possible, we can create a new branch from latest release tag and cherry pick this PR there, and release patch version from that branch.