Check existence of node.nodes in hasNoDeclarations

[chrisnicotera]

While trying to use the stylelint-selector-bem-pattern plugin (which uses postcss-bem-linter under the hood) in tandem with stylelint, using less syntax supplied by postcss-less, I noticed that the linter would error upon trying to parse mixins (as reported in #104), i.e.:

Contrived, breaking code:

/** @define MyComponent */
.mixin {
    background: black;
}

.MyComponent {
    .mixin;
    &:extend(.mixin);
}

yields:

TypeError: Cannot read property 'length' of undefined
    at hasNoDeclarations (node_modules\postcss-bem-linter\lib\get-selectors.js:12:20)
    at getSelectors (node_modules\postcss-bem-linter\lib\get-selectors.js:39:7)
    at module.exports (node_modules\postcss-bem-linter\lib\validate-selectors.js:27:19)
    at checkRule (node_modules\postcss-bem-linter\index.js:80:7)
    at node_modules\postcss-bem-linter\index.js:47:9
    at Array.forEach (native)
    at node_modules\postcss-bem-linter\index.js:44:14
    at node_modules\postcss\lib\container.js:241:28
    at node_modules\postcss\lib\container.js:148:26
    at Root.each (node_modules\postcss\lib\container.js:114:22)

After existence check, yields proper output:

input.less
3:1     ‼  Invalid component selector ".mixin" (plugin/selector-bem-pattern) [stylelint]
8:3     ‼  Invalid component selector ".MyComponent .mixin" (plugin/selector-bem-pattern) [stylelint]

I know that this project is not less (or other pre-processor)-specific, but a seemingly innocuous change of checking for the existence of node.nodes before accessing .length resolves this issue. This fix would also resolve #104.

As far as the history of this issue: it looks like it can be sort of traced back to postcss-less#33, where a decision was made to mimic the behavior of postcss-scss, which sets node.nodes to undefined when there are no children of a given rule. I can't speak to the how or why of that, but it's arguable about whether or not a discussion/fix belongs in a different project as well.

[simonsmith]

This is a good catch. Thanks for the detail on the issue @chrisnicotera

I wonder if we should a test for this? Maybe with just the code example you added.

[chrisnicotera]

@simonsmith I thought about adding a test to this, but ultimately I think this is a pre-processor specific detail. Given that this plugin deals specifically with css and bem and the fact that this implementation detail is not valid css, I didn't want to add something non-standard just to accommodate that. Thoughts?

[simonsmith]

I see your point.

My reasoning is we do already have non standard tests (see the nesting fixtures) and ideally any change added should have some form of test to validate it and protect us against regressions later.

[chrisnicotera]

I agree and understand your reasoning. Is the added test acceptable?

[simonsmith]

Looks good to me. Thanks for this fix!

[chrisnicotera]

@simonsmith Not sure what your release process/cycle/free time is like, but any chance of a 2.7.1 soon? :)

[simonsmith]

@chrisnicotera Whoops. Of course. I'll do it in an hour or so

[simonsmith]

Done

[chrisnicotera]

Much thanks for the quick turnaround!