css-modules / postcss-modules-local-by-default

PostCSS plugin for css modules to local-scope classes and ids
MIT License
25 stars 13 forks source link

pure selectors mark all children pure #72

Open jantimon opened 3 months ago

jantimon commented 3 months ago

follow up on #64

adds more valid cases for nested css:

a case which I could not solve (and would probably be a separate pr) is the following one:

alexander-akait commented 3 months ago

Let's solve them here

html { .foo { a_value: some-value; } } should be pure html { a_value: some-value; .foo { a_value: some-value; } } must stay non-pure

Otherwise it can be a problem for some developers

jantimon commented 3 months ago

You are absolutely right - but maybe you got me wrong.

html { .foo { a_value: some-value; } } should be pure html { a_value: some-value; .foo { a_value: some-value; } } must stay non-pure

That part is unchanged - same as on master. So it will stay the very same problem for some developers as it already is today.

In this pr I only improved the mentioned cases because they are more common and easier to detect.

To fix the unchanged case above we must change the entire logic how we detect pure selectors.
I created the issue #73 so we can discuss how we can solve it.

For now please lets merge already this part as it is independent, it works and will already unblock these cases

alexander-akait commented 3 months ago

I don't sure we have a valid logic here, because

.foo { span { a_value: some-value; } }

===

.foo span{a_value:some-value}

but the second is not pure

Why do you think your examples are valid?

jantimon commented 3 months ago

Oh wow - it looks you are absolutely right - I need some time to analyse what's wrong here

alexander-akait commented 3 months ago

Feel free to ping me

jantimon commented 3 months ago

@alexander-akait

Why do you think your examples are valid?

Because they are valid according to postcss-modules-local-by-default see #74

alexander-akait commented 3 months ago

hm, I tried to find any docs about what is pure and no luck, look like I need to look at the source code, anyway we should finish

html { .foo { a_value: some-value; } } should be pure

html { a_value: some-value; .foo { a_value: some-value; } } must stay non-pure

Before merge, otherwise logic will be completed and can create dangerous problems, developers can start to write wrong things and after fix them some project can be broken

jantimon commented 3 months ago

I added a test for this case which shows that

html { a_value: some-value; .foo { a_value: some-value; } } stays non-pure

However

html { .foo { a_value: some-value; } } is not pure (same as on master)

So we don't allow something which will be broken in future

jantimon commented 2 months ago

@alexander-akait lightning is currently working on something similar: https://github.com/parcel-bundler/lightningcss/pull/796

alexander-akait commented 1 week ago

@jantimon Sorry for the delay, I missed your answer, can you list the unsolved cases, I will help with them

jantimon commented 1 week ago

the only missing case would be this one:

html { .foo { a_value: some-value; } } should be detected as pure but is not pure right now however it would mean that we switch from rule based detection to declaration based detection which would probably be a big refactoring - to give you a first impression take a look at #77 which adds declaration to all test scenarios

can we first merge #74 to have better tests? and #72 as it already adds more value and is closer to lightning css again?