Skip to content

Conversation

@jantimon
Copy link
Contributor

@jantimon jantimon commented Jul 9, 2024

follow up on #64

adds more valid cases for nested css:

  • a test for .foo { span { a_value: some-value; } } which is pure
  • a test for .foo { span { a { a_value: some-value; } } } which is pure
  • a test for .foo, html { span { a_value: some-value; } } which is not pure

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

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

@jantimon jantimon force-pushed the feature/pure-parents branch 4 times, most recently from 47212eb to 11d7986 Compare July 9, 2024 19:01
@jantimon jantimon force-pushed the feature/pure-parents branch from 11d7986 to 55d9b43 Compare July 9, 2024 19:05
@alexander-akait
Copy link
Member

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
Copy link
Contributor Author

jantimon commented Jul 17, 2024

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
Copy link
Member

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
Copy link
Contributor Author

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

@alexander-akait
Copy link
Member

Feel free to ping me

@jantimon
Copy link
Contributor Author

jantimon commented Aug 6, 2024

@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
Copy link
Member

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 jantimon force-pushed the feature/pure-parents branch from 9befc8c to 6cea2d6 Compare August 6, 2024 15:18
@jantimon
Copy link
Contributor Author

jantimon commented Aug 6, 2024

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
Copy link
Contributor Author

@alexander-akait lightning is currently working on something similar:
parcel-bundler/lightningcss#796

@alexander-akait
Copy link
Member

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

@jantimon
Copy link
Contributor Author

jantimon commented Oct 25, 2024

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?

@alexander-akait
Copy link
Member

alexander-akait commented Nov 5, 2024

@jantimon Let's add only test cases (more the better 😄 ) here and I will finish it tomorrow and we will make release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants