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

[Bug] : CSSNano has problems with postcss-nested #1004

Open
Chris2011 opened this issue Feb 26, 2021 · 10 comments
Open

[Bug] : CSSNano has problems with postcss-nested #1004

Chris2011 opened this issue Feb 26, 2021 · 10 comments
Labels
Milestone

Comments

@Chris2011
Copy link

@Chris2011 Chris2011 commented Feb 26, 2021

Describe the bug
I think it is a bug but at the end I can understand, that this is a missing feature maybe.

I have a piece of code, in which I have two properties with the same value margin-top: 60px in a p and in an id selector. CSSNano can handle this and put this together to #app,p{margin-top: 60px}. It will remove the separated p and the property inside of the #app selector and merge them together.

When I use the plugin postcss-nested and I put the div within the #app selector, cssnano will not remove the duplicated value.
It will just put the p on the same level, which is correct #app p{margin-top:60px} but it will leave the margin-top also inside of the app. For me, I expect to have this #app,#app p{margin-top:60px}. Everything what I described, you can find here: https://github.com/Chris2011/cssnano-postcss-nested-problem

To Reproduce
Steps to reproduce the behavior:

  1. Clone repo: https://github.com/Chris2011/cssnano-postcss-nested-problem
  2. Do a yarn
  3. Run yarn build
  4. Go to the dist\assets folder and open the css file.
  5. Result is: #app{margin-top:60px}#app p{margin-top:60px}

Expected behavior
Result should be: #app,#app p{margin-top:60px}

Desktop (please complete the following information):

  • CSSNANO Version [e.g. 22]: 4.1.10
  • Plugins/presets versions: plugin: postcss-nested - 5.0.3, preset: default
  • Add the output of npx envinfo && npm ls cssnano
  System:
    OS: Windows 10 10.0.19042
    CPU: (8) x64 Intel(R) Core(TM) i7-1065G7 CPU @ 1.30GHz
    Memory: 2.58 GB / 15.60 GB
  Binaries:
    Node: 12.16.3 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.10 - ~\AppData\Roaming\npm\yarn.CMD
    npm: 7.5.2 - C:\Program Files\nodejs\npm.CMD
  Managers:
    Gradle: 6.5 - C:\Gradle\gradle-6.5\bin\gradle.BAT
    pip3: 19.2.3 - C:\Python38\Scripts\pip3.EXE
  Utilities:
    Git: 2.30.1.
  Virtualization:
    Docker: 20.10.2 - C:\ProgramData\DockerDesktop\version-bin\docker.EXE
  IDEs:
    VSCode: 1.53.2 - C:\Users\Chris\AppData\Local\Programs\Microsoft VS Code\bin\code.CMD
    Visual Studio: 16.8.30711.63 (Visual Studio Community 2019)
  Languages:
    Bash: 4.4.20 - C:\Windows\System32\bash.EXE
    Go: 1.14.4 - C:\Go\bin\go.EXE
    Python: 3.8.2
  Databases:
    SQLite: 3.32.2 - C:\Users\Chris\AppData\Local\Android\Sdk\platform-tools\sqlite3.EXE
  Browsers:
    Chrome: 88.0.4324.182
    Edge: Spartan (44.19041.423.0), Chromium (88.0.705.74)
    Internet Explorer: 11.0.19041.1

cssnano-postcss-nested-problem@0.0.0 C:\Projekte\Others\vite-project
-- cssnano@4.1.10
@ludofischer
Copy link
Collaborator

@ludofischer ludofischer commented Mar 7, 2021

Hi, thank you for the detailed reproduction! I have managed to reproduce the bug and I also believe it is fixed in the cssnano 5.0.0 release candidate. Could you tell me if the release candidate fixes the issue on your end too?
To test, set the cssnano version in package.json to

"cssnano": "^5.0.0-rc.1",

@ludofischer ludofischer added this to the 5.0.0 milestone Mar 7, 2021
@Chris2011
Copy link
Author

@Chris2011 Chris2011 commented Mar 7, 2021

Hey, great to hear it. I tested it, and it seems to work. thx :)

@Chris2011 Chris2011 closed this Mar 7, 2021
@Chris2011
Copy link
Author

@Chris2011 Chris2011 commented Mar 7, 2021

Unfortunately, I have a more detailed example:

#app {
  font-family: Avenir, Helvetica, Arial, sans-serif;
  -webkit-font-smoothing: antialiased;
  -moz-osx-font-smoothing: grayscale;
  text-align: center;
  color: #2c3e50;
  margin-top: 60px;
  transform: scale(0.5);
  transition: all 400ms ease-in;

  p {
    margin-top: 60px;
    width: 100%;
    height: 50px;
    display: block;
    background-color: yellow;
    border: 1px solid black;

    &.t {
      color: green;
    }
  }
}

div {
  margin-top: 60px;
}

and the output ignores the div it is still extra at the end of the file, but I expect that it will be merged togehter with the other selectors with margin-top: 60px;

Output:

#app {
  -webkit-font-smoothing: antialiased;
  -moz-osx-font-smoothing: grayscale;
  color: #2c3e50;
  font-family: Avenir, Helvetica, Arial, sans-serif;
  text-align: center;
  transform: scale(0.5);
  transition: all 0.4s ease-in;
}
#app,
#app p {
  margin-top: 60px;
}
#app p {
  background-color: #ff0;
  border: 1px solid #000;
  display: block;
  height: 50px;
  width: 100%;
}
div {
  margin-top: 60px;
}

@Chris2011 Chris2011 reopened this Mar 7, 2021
@ludofischer
Copy link
Collaborator

@ludofischer ludofischer commented Mar 9, 2021

Interesting. My intuition would be that cssnano runs before the rules get unnested, so it does not 'see' them, but that seems unlikely as the latest cssnano uses OnceExit() and should run after postcss-nested which uses Rule

@Chris2011
Copy link
Author

@Chris2011 Chris2011 commented Mar 9, 2021

Yeah I thought also. I thought I need to change the order, when the stuff should run, but this is not needed as you said?

@Chris2011
Copy link
Author

@Chris2011 Chris2011 commented Apr 1, 2021

hey, is there anything todo where I can help here?

@ludofischer
Copy link
Collaborator

@ludofischer ludofischer commented Apr 2, 2021

I don't think there is anything to do except diving inside the postcss-merge-rules code and trying to understand where it goes wrong. This issue is not the worst either, there are multiple cases where merging the rules results in incorrect output: #1006, #701, #1000, #999 So I think that plugin is in need of a close look, but I am not familiar with the algorithm it uses.

@Chris2011
Copy link
Author

@Chris2011 Chris2011 commented Apr 4, 2021

Hey @ludofischer thx for the info. That's is all what I wanted to know. Thx :)

@ludofischer ludofischer removed this from the 5.0.0 milestone Apr 13, 2021
@ludofischer ludofischer added this to the backlog milestone Apr 13, 2021
@ludofischer ludofischer added this to To do in merge-rules bugs Apr 28, 2021
@Chris2011
Copy link
Author

@Chris2011 Chris2011 commented Sep 28, 2021

If I have time, I can also have a look into it. Maybe adding console.logs into it to see what comes first :D

@Chris2011
Copy link
Author

@Chris2011 Chris2011 commented Sep 28, 2021

Didn't check the new version yet, will check this first and will let you know :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants