Validate nested rulesets

[simonsmith]

A first stab at #77

It only supports one level of nesting, which seems ideal for pseudo classes and such. It does mean things like this will fail though:

.Component {
  &--modifier {
    .Component-element {}
}

Not sure if going any further sends the bem linter into postcss-nested territory but if a nested feature lands in the bem linter with caveats it might not be as useful. Open to thoughts on that.

@dryoma @giuseppeg

Happy to add documentation once this is ready

[giuseppeg]

Cool Simon! Before I even take a look at the code I was thinking that maybe we shouldn't allow modifers nesting. Modifiers should be explicit.

/*
 A modifier creates something like a "context" where
 only -children, states etc are allowed.
*/
.Component--modifier { 
  &.is-state {}

  & .Component-child { /* should & be optional here? */
     &.is-active {}
     &:hover {}
     &[attr] {}
  }
}

This may be not trivial to implement but aligns well with the suitcss methodology imho.

[dryoma]

Modifiers should be explicit.

I agree.

As for postcss-nested I am a bit sceptical. stylelint authors recommend to lint code before applying any plugins. I should say, for a reason. I think it's giving way to errors added by those plugins (if they have errors). As an example, just now tried to lint this:

/** @define MyComponent */

.ns-MyComponent {
  display: inline; // the semicolon is lost somewhere between postcss-nested and stylelint

  &.is-open {
    display: none;
  }

  @media (max-width: 1000px) {
    display: table;
  }
}

with

...
  .pipe(gulpPostCSS([
      nested(),
      stylelint(),
      reporter({
        clearMessages: true,
        throwError: true
      })
    ], {
      syntax: scssSyntax
    }))

and it falsely found an error: 4:20 ‼ Expected a trailing semicolon (declaration-block-trailing-semicolon) [stylelint] If I remove the @media rule, the error goes away.

[simonsmith]

Modifiers should be explicit.

Let me see if I understand by example:

.Component {
  color: red;

  &.is-stateName {}
  &-descendant {}
  &:pseudo-class {}
  &:pseudo-element {}
  &::pseudo-element {}
  &[attr] {}
}

/* modifier needs to start a new block? */

.Component--modifier {
  .Component-descendant {}
  &.is-stateName {}
  &:pseudo-class {}
}

/* How about modifiers on descendants? */

.Component {
  &-descendant--modifier {}
}

I also wonder if this whole thing should be recursive, or whether we should allow one level of nesting? It gets complex the more you think about it :)

[simonsmith]

As for postcss-nested I am a bit sceptical

Agreed, it's not ideal. I was just wondering if bem-linter should be implementing almost that whole plugin as once you support nesting users may try to put @media blocks in rulesets a la Sass for example.

Sounds like we would be aiming to allow nesting with specific rules though.

[dryoma]

I get it that it's a PostCSS, not PostSass plugin. Still, I think that it is quite natural for a user to expect postcss-bem-linter to support nesting as well, since it can be used as a plugin to stylelint or alongside it, and stylelint can support SCSS syntax via postcss-scss (and it's especially great it works with nesting) and doesn't need any specific preprocessing plugins for this (like postcss-nesting in question).

Otherwise it would require a separate pipelilne for linting selectors. Not to mention the plugins may cause errors or be in some other way inacurate themselves.

[simonsmith]

@dryoma Agreed yes, but I also wonder if bem-linter should take this chance to lay down some rules and stop the nested hell you sometimes see in Sass and LESS

[davidtheclark]

I think using postcss-resolve-nested-selector might solve some of the complexity concern.

I also think that people who want to restrict nesting depth could use stylelint. I think it's nice to keep this particular plugin focused solely on selectors.

[simonsmith]

@davidtheclark Ah nice! Exactly what I needed. I'll look at implementing that and leave the depth concern to stylelint as you mentioned

[dryoma]

I also think that people who want to restrict nesting depth could use stylelint.

I agree, it would be sensible to let stylelint worry about nesting (which it does perfectly, even with SCSS).

[giuseppeg]

@simonsmith are you just unwrapping and validating the nested rules then? If so I think that the branch is ok!

By the way are you familiar with the CSS Nesting Module Level 3 proposal by Tab Atkins? Would it make sense to try to add support for this too? I mean we are introducing new rules and if we want to move away from them or remove some in the future that'd require a breaking change.

[simonsmith]

@giuseppeg Yeah, I'm thinking do the minimum here which is just unwrapping all the selectors and validating them. Changes to those rules can happen outside this PR.

I'm currently refactoring to use postcss-resolve-nested-selector (via @davidtheclark) which supports postcss-nested (Sass) and postcss-nesting (CSS3) so that should cover it.

Will push some changes for review shortly.

[simonsmith]

Okay, pushed some changes.

The idea now is to detect if a rule is nested and if so, traverse up to the component root rule and construct the selector from there down to the current rule. The reason for this is so we always have the correct rule to output as the warning if linting fails.

See what you think

[simonsmith]

@davidtheclark Look okay to you?

[davidtheclark]

Just had a couple of comments I added.

[simonsmith]

I've pushed some more changes and added some more tests. Main change was to move the logic into a getSelectors function so it could be isolated in the tests. I'll post my commit message as an explanation:

This allows certainty that selectors are being unwrapped as expected. By
following the 'black box' style used in other tests it can lead to false
positives. For example:

.Component {
  .Component-descendant {
    color: green;
  }
}

In the above code what should be linted is
`.Component .Component-descendant` but it's quite possible due to an
error that just `.Component` is tested. If this happens it will still
pass the black box test as `.Component` is valid. This can lead to hard
to find bugs with unwrapping nesting selectors.

The extra tests give it a bit more confidence that this is working as expected. Have also added some CSS Nesting Module fixtures.

[davidtheclark]

Looks good! Just that one comment. Anybody object to merging after it's addressed?

[giuseppeg]

Looks good to me, feel free to ignore my last round of comments if you want ;)

[davidtheclark]

Thanks all. I'm fine with merging, so ping me when you think it's ready @simonsmith.

[simonsmith]

@davidtheclark I'm happy with this now. Commits are pretty tidy. Need anything added to the README.md?

[davidtheclark]

I think it's fine. Released in 2.5.0! Thanks again everybody!

[simonsmith]

🎉

[dryoma]

Great, thanks @simonsmith !

[marcustisater]

Wohoo! Great work @simonsmith! 🎉 😄

[brianmcallister]

Little late to the party, but this doesn't appear to be working for me in v2.6.0. I am using stylelint-selector-bem-pattern, but I've installed 2.6.0 of this package specifically. If you think this is more of a problem with the stylelint plugin, I'd be happy to post this over there.

Here's my Sass file:

_test.scss

/* postcss-bem-linter: define test */
.test {
  &--red {
    color: rgba(255, 0 0);
  }

  .child {
    color: #ffffff;
  }
}

and here's my config:

module.exports = {
  plugins: [
    'stylelint-selector-bem-pattern'
  ],
  rules: {
    'plugin/selector-bem-pattern': {
      componentName: /[-a-z]+$/,
      componentSelectors: '^\\.{componentName}(?:[-_a-z]+)?$'
    },
  }
};

and my output:

src/stylesheets/modules/_test.scss
 7:3  ✖  Invalid component selector ".test .child"   plugin/selector-bem-pattern

Am I just doing something wrong? I started with a super simple regex for now, just to validate that this works.

[simonsmith]

Hey @brianmcallister,

Do you mind adding that as a separate issue so I don't lose track of it. Thanks

[brianmcallister]

Yep! No problem.