Skip to content

Conversation

@kdy1
Copy link
Contributor

@kdy1 kdy1 commented Aug 14, 2024

Copy link
Contributor

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some more tests, the current test only covers the top level but there's other case where it shouldn't error.

Examples:

  • div .my-class -> should not error
  • .my-class div -> should not error
  • div.my-class -> should not error

These are the cases I tried that compile fine with webpack CSS Modules

div.my-class {
  color: red;
}

a .my-class {
  color: red;
}

.my-class a {
  color: red;
}

@kdy1 kdy1 marked this pull request as draft August 15, 2024 01:46
@kdy1 kdy1 marked this pull request as ready for review August 16, 2024 00:35
@kdy1 kdy1 requested a review from timneutkens August 16, 2024 00:35
Copy link
Contributor

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now, thanks for adding the additional tests 💯

Copy link
Contributor

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Realized this is not documented yet, can you make sure there's documentation added as well, it's in this repo in the website folder 👍

@kdy1 kdy1 requested a review from timneutkens August 19, 2024 10:38
@jantimon
Copy link
Contributor

Could we please also add tests for nesting?
Because that's buggy right now in https://github.com/css-modules/postcss-modules-local-by-default

should not error:

div {
  .my-class {
     color: red; 
  }
}
div {
  .my-class {
     span {
       color: red; 
     }
  }
}

should error:

div {
  .my-class {
     color: red; 
  }
  color: blue
}

@devongovett
Copy link
Member

What should happen for cases like .my-class:is(a)? Currently that errors, presumably while parsing the inner selector inside the :is. If this selector had been written as a.my-class it would not error so seems like it probably shouldn't in the :is case also?

Copy link
Contributor Author

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I introduced a new selector state flag and I resolved the test case you mentioned.

@kdy1
Copy link
Contributor Author

kdy1 commented Aug 29, 2024

I think html { .foo { span { color: red; } } } is nearly impossible to handle with the current model

@kdy1
Copy link
Contributor Author

kdy1 commented Aug 29, 2024

We are detecting from selector passer, not AST visitor

@jantimon
Copy link
Contributor

I see - checking only the selector has worked very well for many years
Unfortunately css nesting changed that..

this test case works in nextjs using postcss but never will in lightningcss
https://stackblitz.com/edit/stackblitz-starters-ywc6pu?file=app%2Fpage.module.css

@devongovett
Copy link
Member

I think this should not be allowed:

html {
  .foo {
    /* ... */
  }
}

because it's easy to start adding declarations to the root html rule itself which is not pure.

However the inverse seems ok:

.foo {
  div {
    /* ... */
  }
}

@devongovett
Copy link
Member

Updated to implement this in the transform pass rather than in the selector parser, which enables handling of nesting as shown above.

@devongovett devongovett merged commit 22ef664 into parcel-bundler:master Aug 31, 2024
@jantimon
Copy link
Contributor

thanks @kdy1 and @devongovett for all your work!

@timneutkens
Copy link
Contributor

timneutkens commented Sep 3, 2024

Thanks @kdy1 @devongovett @jantimon, great collaboration 😌

kdy1 added a commit to vercel/next.js that referenced this pull request Sep 12, 2024
### What?

Update `lightningcss`

### Why?

To apply parcel-bundler/lightningcss#796 and
parcel-bundler/lightningcss#797

### How?

 - Closes PACK-3190
 - Closes PACK-3191
@kdy1 kdy1 deleted the lint-pure branch September 20, 2024 02:55
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.

4 participants