Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pure selectors mark all children pure #72

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

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
@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
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