postcss / postcss-bem-linter

A BEM linter for postcss
MIT License
572 stars 35 forks source link

Issue with prepending selectors (separated by whitespace) #130

Closed saulhardman closed 6 years ago

saulhardman commented 6 years ago

Hi @simonsmith, first of all, thanks a lot for maintaining this project 🙇

We're using SCSS syntax on the project in question and I'm attempting to extend the componentSelectors RegExp to allow for 'global state' classes to be prepended. E.g. .no-js .c-responsive-image etc.

The regular expression that I've conjured up appears to do the job when run separately, but it's outputting warnings when used with BEM Linter.

A (relatively) reduced test case can be found here on Glitch.

/** @define responsive-image */
.c-responsive-image {
  display: block;

  .no-js & {
    display: none;
  }
}

/* without `&` */
.no-js .c-responsive-image {
  display: none;
}
const { join } = require('path');
const { promisify } = require('util');
const readFile = promisify(require('fs').readFile)

const postcss = require('postcss');
const bemLinter = require('postcss-bem-linter');
const reporter = require('postcss-reporter');

const kebabCaseString = '[a-z]+[-[a-z]+]*';
const kebabCaseStringWithNumbers = '[a-z]+[-[a-z0-9]+]*';
const globalStateClassNames = `\\.(no|has|supports|t)-${kebabCaseString}`;
const localStateClassNames = `\\.(is|has)-${kebabCaseString}`;

const componentName = `^${kebabCaseString}$`;
const componentSelectors = name => new RegExp(`^(${globalStateClassNames}\\s)?\\.[co]-${name}(_{2}${kebabCaseString})?(-{2}${kebabCaseString})?(${localStateClassNames})?$`);
const utilitySelectors = `^\\.u-${kebabCaseStringWithNumbers}$`;

const bemLinterOptions = {
  componentName,
  componentSelectors,
  utilitySelectors,
};

console.log('Standalone RegExp test:', componentSelectors('responsive-image').test('.no-js .c-responsive-image'));

module.exports = (async () => {
  const scss = await readFile(join(process.cwd(), 'index.scss'), 'utf8');

  await postcss()
    .use(bemLinter(bemLinterOptions))
    .use(reporter())
    .process(scss)
})();

Can you spot anything obvious that I'm doing wrong? If this is in fact a bug then I'm more than willing to contribute to the fix, but I'll need a little guidance.

Edit:

# error output
6:3 ⚠  Invalid component selector ".no-js .c-responsive-image" [postcss-bem-linter]
5:50 PM
12:1    ⚠  Invalid component selector ".no-js .c-responsive-image" [postcss-bem-linter]
simonsmith commented 6 years ago

Thanks for the detailed report!

The regular expression that I've conjured up appears to do the job when run separately, but it's outputting warnings when used with BEM Linter.

Are you saying the plugin appears to be ignoring your regex?

saulhardman commented 6 years ago

Thanks for the quick response!

It's not that the plugin is ignoring it, more that it appears to be giving me a false negative.

simonsmith commented 6 years ago

I'll try your example later and report back

saulhardman commented 6 years ago

Thanks @simonsmith 👍

simonsmith commented 6 years ago

@saulhardman Do I need to join the Glitch project to access the console?

saulhardman commented 6 years ago

@simonsmith unsure – it's the first time I've taken it for a spin. I've accepted your request to join.

simonsmith commented 6 years ago

I've had a look at the source code with your example (I haven't looked for a while!):

The exported function in validate-selectors.js is where your componentSelectors function ends up as selectorPattern. That is used to generate the initialPattern regex which looks like:

^(\.(no|has|supports|t)-[a-z]+[-[a-z]+]*\s)?\.[co]-responsive-image(_{2}[a-z]+[-[a-z]+]*)?(-{2}[a-z]+[-[a-z]+]*)?(\.(is|has)-[a-z]+[-[a-z]+]*)?$

Line 54 Runs your regex against each 'sequence' in the selector. In this case the sequences are:

[ '.no-js', '.c-responsive-image' ]

If I run the generated regex against both of those sequences it only passes for .c-responsive-image as seen here - https://regex101.com/r/SmL12u/2

The README.md says

initial describes valid initial selector sequences — those occurring at the beginning of a selector, before any combinators.

So perhaps try to adjust your regex accordingly to be able to run against each sequence. I admit the plugin itself could make this far clearer!

Hope that helps. We can always improve things if you think this is not working as you expected or not very clear

saulhardman commented 6 years ago

Hey @simonsmith, thanks very much for looking into this :+1:

It just shows my ignorance with respect to the meaning behind and function of the 'initial' and 'combined' selectors. The terms are pretty confusing, so perhaps walkthrough example would be helpful?

I've separated the 2 regexes now and appear to have gotten to a satisfactory point.

For posterity, the following hastily constructed and thoroughly untested RegExp allows the following selector format:

[.no-js.supports-intersection-observer] .c-card[__body][--large][.is-active.is-highlighted]
const { oneLineTrim } = require('common-tags');

const kebabCaseString = '[a-z]+(?:-[a-z]+)*';
const kebabCaseStringWithNumbers = '[a-z]+(?:-[a-z0-9]+)*';
const globalStateClassNames = `(?:\\.(?:no|has|supports|t)-${kebabCaseString})+`;
const localStateClassNames = `(?:\\.(?:is|has)-${kebabCaseString})+`;
const componentSelector = name => oneLineTrim`
  \\.[co]-${name}
  (?:_{2}${kebabCaseString})?
  (?:-{2}${kebabCaseString})?
  (?:${localStateClassNames})?
`;

const componentName = `^${kebabCaseString}$`;
const componentSelectors = {
  initial: name => new RegExp(oneLineTrim`^
    (?:${globalStateClassNames})?
    (?:${componentSelector(name)})?
  $`),
  combined: name => new RegExp(oneLineTrim`^
    (?:${globalStateClassNames}\\s)*
    ${componentSelector(name)}
  $`),
};
const utilitySelectors = `^\\.u-${kebabCaseStringWithNumbers}$`;

// .stylelintrc.js
module.exports = {
  plugins: [
    'stylelint-selector-bem-pattern',
  ],

  rules: {
    'plugin/selector-bem-pattern': {
      componentName,
      componentSelectors,
      utilitySelectors,
    },
  },
};

// using PostCSS
postcss()
  .use(bemLinter({
    componentName,
    componentSelectors,
    utilitySelectors,
  }))
  .use(reporter())
simonsmith commented 6 years ago

@saulhardman Good to hear! Glad it works

I agree the documentation is very light on initial vs combined. I think some code examples are needed there.

If you're happy this is resolved we can close this and I'll track the documentation update in a different issue

saulhardman commented 6 years ago

Yes, most definitely, thanks again for the help Simon 🙇 👏 👍