search

postcss / postcss-nested

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

Issue with the nested styles (third level) #125

Closed naxxateux closed 3 years ago

naxxateux commented 3 years ago

Hi!

Just updated postcss-nested to v. 5.0.5 (from 5.0.3) and got this issue.

This is my .sss file:

...
.text
  ...
  &:hover
    border-color: red
      &:before
        color: red
  ...

And I got this as a result:

.text:hover {
    border-color: red;
    &: :before {;
    color: red;
    }: ;
}

What seems to be the problem here? And how can I fix it properly? Thnx in advance.

ai commented 3 years ago

Seems like a strange bug in the new PostCSS 8 event system.

I will have a relocation this week, so if you want to fix it quicker, try to investigate what is happening here https://github.com/postcss/postcss-nested/blob/main/index.js#L143-L155

naxxateux commented 3 years ago

@ai thnx for the quickest response ever :-) I will try to investigate it next week.

TheSeally commented 3 years ago

@naxxateux, I can't reproduce this issue. I tried it on windows and ubuntu with different postcss parsers and bundlers, but css was always processed correctly. Can you add additional info about your environment or maybe try to reproduce issue again?

ai commented 3 years ago

There is also similar issue here https://github.com/postcss/postcss-mixins/issues/132

ai commented 3 years ago

And seems like here we have similar issue https://github.com/postcss/postcss-nested/issues/129

TheSeally commented 3 years ago

I reproduced issue. It is very interesting that with postcss-nested@5.0.5 and postcss@8.3.0 there is no problem, but with postcss@8.3.1 (or on version lower than 8.3) issue happened. I'll debug it and try to make PR for fix

ai commented 3 years ago

@TheSeally very, very strange. postcss@8.3.1 changes only no plugins or parser warning detection.

Maybe node_modules structure somehow creates a bug.

exdeniz commented 3 years ago

I added tests and check on these versions of postcss: 8.3.2 8.3.1 8.3.0 8.2.15 8.2.0 8.1.3 and and all version postcss-selector-parser@6.0.x, and node 10.21, 12.18.2, 14.10.1, 16.3.0 All test passed.

ai commented 3 years ago

@exdeniz it means that issue is with some another component. Maybe PostCSS?

exdeniz commented 3 years ago

@ai I had a suspicion that maybe postcss-nested was preceded by postcss-present-env with the postcss-nesting plugin. But I checked it out and it didn't check out.

ai commented 3 years ago

Hm. We can add a warning to postcss-nested if we will find postcss-nesting in result.processor.plugins

ai commented 3 years ago

@naxxateux did we have postcss-preset-env in Amplifr?

exdeniz commented 3 years ago

@naxxateux And you can get a config PostCSS?

TheSeally commented 3 years ago

I reproduced it in https://github.com/ai/sitnik.ru project, but it reproduced only when I use yarn

Steps:

  1. clone and install deps via yarn (now postcss version is 8.3.0)
  2. add nested styles
  3. build project (now css processed corectly)
  4. update postcss to 8.3.1 via yarn
  5. try to build again (now styles compile with problem. also bug with media queries syntax with < > operators)

When I install deps via npm there is no problem :)

ai commented 3 years ago

😅 Very strange bug.

But a few people told me that they have a bug after PostCSS update. Can you find a source of problem in a broken environment?

TheSeally commented 3 years ago

yeah I will try to do it

TheSeally commented 3 years ago

I debuged a little bit and found that problem can connect with different postcss version in deps.

When I install postcss@8.3.0 with sugarss via yarn I have only one postcss in deps folder, but when I install postcss@8.3.1 (via yarn) I have two different postcss in deps (one standalone package and one in sugarss folder). So because postcss use Symbol isClean for checking root than when we have 2 different postcss packages we have two different symbols isClean that are not equal.

We can use global Symbol via Symbols.for in symbols.js file in postcss this will fix problem with different postcss versions, but I don't know is it good decision. https://github.com/postcss/postcss/blob/2da5501f709862c19e7103b81ba8fb224a5793ff/lib/symbols.js#L3

@ai what do you think about this?

ai commented 3 years ago

Wow, great! It looks very obvious now.

I will think about solution. Maybe it is better to switch to string key.

I will mark you as a solver for Cult of Martians task. What is your Twitter account for future @postcss tweet?

TheSeally commented 3 years ago

Thanks, my twitter link https://twitter.com/autumn_poet

ai commented 3 years ago

@TheSeally I made 2 fixes:

  1. Long term: require parsers to have postcss in peerDependencies (but it will take a few weeks to release new versions) https://github.com/postcss/postcss/commit/714c5c6263f5930a5ef0c1704bbc707db9581ed5
  2. Short term: detect broken AST and update prototypes https://github.com/postcss/postcss/commit/d8edfeda3804a63d81d010858ade6ce5e49b93a3

Can I ask you to test the second fix?

  1. Reproduce error
  2. Replace (in standalone postcss) node_modules/postcss/container.js, node_modules/postcss/lazy-result.js and node_modules/postcss/symbols.js with a version from GitHub’s main branch.
  3. Check issue again
TheSeally commented 3 years ago

@ai after replacing scripts in standalone package, css is processed correctly

ai commented 3 years ago

Fixed with PostCSS 8.3.3.

naxxateux commented 3 years ago

Guys, thnx for investigation and fix. Sorry for not responding — I was on vacation :-)