maximkoretskiy / postcss-autoreset

PostCSS plugin for automatic rules isolation
MIT License
244 stars 11 forks source link

suit regexp is weak #16

Closed giuseppeg closed 7 years ago

giuseppeg commented 7 years ago

Hi, I know that regexps are expensive but the one for suit is very flaky and weak.

For instance the plugin resets things like

.Component[aria-hidden="true"] {}

which results in specificity issues e.g.:

.Component { 
  font-size: 3em;
}

.Component[aria-hidden="true"] {
  display: none;
}

Is compiled to

.Component,
.Component[aria-hidden="true"] {
   font-size: medium;
}

.Component { 
  font-size: 3em;
}

.Component[aria-hidden="true"] {
  display: none;
}

and because .Component[aria-hidden="true"] is more specific than .Component font-size: 3em is overridden.

You may want to use postcss-bem-linter's regexps.

We are considering to add this plugin to suitcss and this is a blocker. Let me know if you need some help ✌️

giuseppeg commented 7 years ago

@maximkoretskiy ping

maximkoretskiy commented 7 years ago

Hi. Sorry for long response. I will be able to implement it next on next week end. Would U like to make PR to get this feature faster?

giuseppeg commented 7 years ago

@maximkoretskiy no worries, thank you! I'll let you know if I find the time to work on it but otherwise feel free to start if you can/want/have time ;)

simonsmith commented 7 years ago

@giuseppeg Should attributes with a selector always be ignored? Is this the only bug or is there a list of rules to write against here?

I can take a look at this

giuseppeg commented 7 years ago

@simonsmith the only things to reset are component roots and descendants I suppose, everything else should be ignored.

.Component {}
.namespace-Component {}
.Component-descendant {}
.namespace-Component-descendant {}

So yes attributes with a selector should be ignored because they will always be more specific than the above.

One may be tempted to add the attribute selector case to the current check but keep in mind that this plugin can run on anything (utilities files, resets and non-suit compliant code) so we really want to make sure that we reset only components classes.

simonsmith commented 7 years ago

I see, okay.

We can safely ignore pseudo classes too? Current version checks for presence of : anywhere in the selector

giuseppeg commented 7 years ago

I guess so. When this is fixed I think that we should approach this integration in a tdd way to make sure that we don't ship something broken or flaky in preprocessor v3 – but this has nothing to do with Maxim's plugin.