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

Trailing comma in selector causes error with no-descending-specificity and Less syntax #1156

Closed
eddiemoore opened this issue Apr 27, 2016 · 9 comments
Assignees
Labels
status: wip is being worked on by someone syntax: less relates to Less and Less-like syntax type: enhancement a new feature that isn't related to rules

Comments

@eddiemoore
Copy link

Found a bug when running stylelint against a bunch of less files.

Error with: no-descending-specificity

Hard to say really what part of the code is causing this issue, as i'm running it agains a HUGE amount of files.

Stylelint configuration:
npm install stylelint-config-migme

{
  "extends": "stylelint-config-migme"
}

Running with version 6.2.2 of stylelint

Running it with CLI with stylelint app/styles/* --syntax less

I believe it may relate to non-standard styntax because i'm running it against .less files

Expected warnings to be flagged and not causing it to crash.

The following warnings were flagged:

TypeError: Cannot read property 'filter' of undefined
    at lastCompoundSelectorWithoutPseudo (/XXX/node_modules/stylelint/dist/rules/no-descending-specificity/index.js:93:53)
    at checkSelector (/XXX/node_modules/stylelint/dist/rules/no-descending-specificity/index.js:34:39)
    at Processor.func (/XXX/node_modules/stylelint/dist/rules/no-descending-specificity/index.js:26:20)
    at Processor.process (/XXX/node_modules/postcss-selector-parser/dist/processor.js:31:14)
    at /XXX/node_modules/stylelint/dist/rules/no-descending-specificity/index.js:27:14
    at Array.forEach (native)
    at /XXX/node_modules/stylelint/dist/rules/no-descending-specificity/index.js:24:69
    at Array.forEach (native)
    at /XXX/node_modules/stylelint/dist/rules/no-descending-specificity/index.js:20:22
    at /XXX/node_modules/postcss/lib/container.js:110:28
@alexander-akait
Copy link
Member

@eddiemoore show example of code and next format issue as template

@alexander-akait alexander-akait added status: needs discussion triage needs further discussion syntax: less relates to Less and Less-like syntax labels Apr 27, 2016
@davidtheclark
Copy link
Contributor

The stack trace is a good indication of what's going wrong but we don't know what sourcecode caused it. The function that's throwing is this:

function lastCompoundSelectorWithoutPseudo(selectorNode) {
  const nodesAfterLastCombinator = _.last(selectorNode.nodes[0].split(node => {
    return node.type === "combinator"
  }))

  const nodesWithoutPseudos = nodesAfterLastCombinator.filter(node => {
    return node.type !== "pseudo"
  })

  return nodesWithoutPseudos.toString()
}

So somehow we're ending up with nodesAfterLastCombinator being undefined or something. We could build safeguards into that function blindly if we're unable to pinpoint the Less that causes it.

@eddiemoore
Copy link
Author

When I get into work tomorrow morning I'll see if I can narrow down the files a bit to see if I can pinpoint the piece of LESS code that's causing the problem

@eddiemoore
Copy link
Author

Ok. I have narrowed it down to a single section.

.selector,
{
  color: red;
}

Seems that if there is a comma at the end of the selectors list then it runs into the error. However LESS still compiles the code with no problems. Would have thought the less compiler should point out this issue.

@alexander-akait
Copy link
Member

@eddiemoore it is not valid CSS, @davidtheclark what do you think we should consider it as an additional check?

@eddiemoore
Copy link
Author

@evilebottnawi Yeah I know it's not valid CSS. It's in legacy code, found it strange that the less compiler even compiled it.

@alexander-akait
Copy link
Member

@eddiemoore waiting what say @davidtheclark about this issue

@davidtheclark
Copy link
Contributor

I think we can try to refactor that function just a little so it is more elegantly handled. Shouldn't be a big deal.

@davidtheclark davidtheclark added status: ready to implement is ready to be worked on by someone type: enhancement a new feature that isn't related to rules and removed status: needs discussion triage needs further discussion labels Apr 28, 2016
@jeddy3 jeddy3 changed the title no-descending-specificity error Trailing comma in selector causes error with no-descending-specificity and Less syntax May 3, 2016
@alexander-akait alexander-akait added status: wip is being worked on by someone and removed status: ready to implement is ready to be worked on by someone labels May 4, 2016
@alexander-akait alexander-akait self-assigned this May 4, 2016
@alexander-akait
Copy link
Member

Close. Merged. Ref: #1181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: wip is being worked on by someone syntax: less relates to Less and Less-like syntax type: enhancement a new feature that isn't related to rules
Development

No branches or pull requests

3 participants