postcss / postcss-nested

PostCSS plugin to unwrap nested rules like how Sass does it.
MIT License
1.16k stars 66 forks source link

empty classes stripped out break postcss-modules #61

Closed FezVrasta closed 7 years ago

FezVrasta commented 7 years ago

Hi, I have this CSS:

.Foo
  &__bar
    color: red

I want to compile it and use postcss-modules to get the classes out of it, it should output:

{
 Foo: 'hash',
 Foo__bar: 'hash',
}

The problem is that this plugin is going to output only:

.Foo__bar { color: red; }

and then postcss-modules isn't able to extract .Foo.

You can reproduce the problem here: https://runkit.com/fezvrasta/59775c35542cda0012cc20ef

Ideas?

ai commented 7 years ago

Hm. #62 will fix only your case. Other users with same problem will not have solution, because it will be hard to find preserveEmpty option.

Any new option is not good UX.

I think the best solution here is to fix postcss-modules to generate hash for every classes in CSS.

Also for you PR to CSS Modules will be more respectful ;).

What do you think? Do you need help with it?

FezVrasta commented 7 years ago

Hi, what do you mean "be more respectful"? I didn't mean to be rude or anything, I just reported a problem I found.

Talking about the problem, how can postcss-modules know the classes it needs to export if they get stripped out (aka, never generated) by this plugin?

ai commented 7 years ago

Hi, what do you mean "be more respectful"?

I mean it is awesome to have commit to CSS Modules :D

ai commented 7 years ago

how can postcss-modules know the classes it needs to export

For .A .B {} rule you just generate hash for .A and for .B. Even if .A doesn’t have own .A {} rule.

FezVrasta commented 7 years ago

My problem is different, I need to export .A even if the only generated class is .A__B.

It's a specific BEM logic and I don't think it fits well inside postcss-modules. That's why I decided to create a PR against this project. (Because it already has all the info it needs)

ai commented 7 years ago

Ouh :(. It is strange and rare case.

Maybe you can add addition plugin after postcss-nested? I am not sure that in this case preserveEmpty will be useful for others.

FezVrasta commented 7 years ago

I can't add anything after postcss-nested because the info is going to be lost.

I think it fits perfectly here because it's a way to tell this plugin to generate all the classes defined in the CSS.

If I have:

.A {
  &__B { color: red; }
  &__C { color: blue; }
}

is duty of this plugin to generate .A {} .A__B {...} .A__C {...}

ai commented 7 years ago

Why lost? Just find X__Y selectors and put empty X {} rules.

FezVrasta commented 7 years ago

What if I have:

.Something {
  &superawesome { }
}

=>

.Somethingsuperawesome {}

?

I don't have any way to know where to split the two classes

ai commented 7 years ago

Why do you need to export Something in this case? :)

FezVrasta commented 7 years ago

Because this is how CSS Modules work (especially in webpack with css-loader).

I just want to preserve all the information generated by this plugin.

ai commented 7 years ago

OK, I change question. Why do you need Something hash? :)

I understnadrd why do you need block class hash. But why do you need hash for every case?

FezVrasta commented 7 years ago

or, for instance. There are people that use different separators for their BEM classes:

.Foo {
 &Bar {}
}

// or
.Foo {
  &_bar {}
}

// or
.Foo {
  &-bar {}
}

and so on. A plugin can't know every possible way an user can split their classes.

I need to export them because if I have this, I want to apply all the needed classes for completeness, clarity and to respect the BEM convention:

.Button
  &--red
    color: red
<button class="Button Button--red" />

BEM doesn't allow <button class="Button--red" />

ai commented 7 years ago

Do you use FooBar selectors?

Does extra PostCSS plugin after postcss-nested covers your case?

FezVrasta commented 7 years ago

At the moment I don't use FooBar selectors and probably a plugin could cover it, but I want to have a solution that will work in the future if we decide to change the way we name classes, or if someone else hits the same problem and looks for a solution to it.

It's a super small change, please just merge it, it's going to help a lot of people.

ai commented 7 years ago

but I want to have a solution that will work in the future

In the future you will fix your plugin, so it will find all FooBar and will create rule for Foo. Will it works?

it's going to help a lot of people.

This is why I am not merging your PR. I think other people with your problem will not find this option. It is a tricky case. And it is hard to understand that there is some option in other plugin.

As result we will reduce maintainability for one-person feature.

ai commented 7 years ago

I thought that maybe we can improve docs to make option more clear

FezVrasta commented 7 years ago

That's perfect, thank you again!

ai commented 7 years ago

Released in 2.1