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

CSSO not restructuring unless media queries are sequencially close to each other in the file. #350

Closed
carlospnav opened this issue Aug 25, 2017 · 8 comments

Comments

@carlospnav
Copy link

I have a project in which I have a couple of different modules, each with their own css file. In my build process I use ExtractTextPlugin to extract the css from the bundle and place it in styles.css. I then tried to use CSSO as a webpack plugin to restructure the resulting styles.css in order to merge media queries sentences to avoid duplicating statements. The result is that the CSSO pass seemed to be processing only the individual chunks and not the complete styles.css. However, upon using the CLI tool on that styles.css separately I got the following results:

With this stylesheet:

body {
  margin: 0;
  padding: 0;
  font-family: sans-serif;
}

@media all and (max-width: 100px){
  .lorelei{
    font-size: 30px;
  }
}

.App {
  text-align: center;
}

.App-logo {
  animation: App-logo-spin infinite 20s linear;
  height: 80px;
}

.App-header {
  background-color: #222;
  height: 150px;
  padding: 20px;
  color: white;
}

.App-intro {
  font-size: large;
}

@media all and (max-width: 100px){
  .loboto{
    font-size: 30px;
  }
}

@media all and (max-width: 100px){
  .jonas{
    font-size: 30px;
  }
}

The resulting minification and restructuring would be this:

body{margin:0;padding:0;font-family:sans-serif}@media all and (max-width:100px){.lorelei{font-size:30px}}.App{text-align:center}.App-logo{animation:App-logo-spin infinite 20s linear;height:80px}.App-header{background-color:#222;height:150px;padding:20px;color:#fff}.App-intro{font-size:large}@keyframes App-logo-spin{0%{transform:rotate(0deg)}to{transform:rotate(360deg)}}@media all and (max-width:100px){.jonas,.loboto{font-size:30px}}

Now, if I repeated the process with the following stylesheet:

body {
  margin: 0;
  padding: 0;
  font-family: sans-serif;
}

.App {
  text-align: center;
}

.App-logo {
  animation: App-logo-spin infinite 20s linear;
  height: 80px;
}

.App-header {
  background-color: #222;
  height: 150px;
  padding: 20px;
  color: white;
}

.App-intro {
  font-size: large;
}


@media all and (max-width: 100px){
  .lorelei{
    font-size: 30px;
  }
}

@media all and (max-width: 100px){
  .loboto{
    font-size: 30px;
  }
}

@media all and (max-width: 100px){
  .jonas{
    font-size: 30px;
  }
}

The resulting minified version would be:

body{margin:0;padding:0;font-family:sans-serif}.App{text-align:center}.App-logo{animation:App-logo-spin infinite 20s linear;height:80px}.App-header{background-color:#222;height:150px;padding:20px;color:#fff}.App-intro{font-size:large}@media all and (max-width:100px){.jonas,.loboto,.lorelei{font-size:30px}} 

Notice that when the two @media all and (max-width: 100px) statements are close to each other, csso will attempt to restructure it to avoid duplications. The same doesn't happen when they are in different locations within the file, but similarly duplicated. Any work-arounds for this problem? If I start having many modules, this can start adding up to the resulting filesize.

@lahmatiy
Copy link
Member

lahmatiy commented Aug 25, 2017

It's a valid behaviour, not a bug. In the first example moving media down is unsafe, since it may change the meaning of your CSS. Let's imaging lorelei and App-intro apply to the same element. In this case, changing the order of rules will produce a differ result than you're authored. You may say for sure those classes are never used on the same element. However, CSSO doesn't know about it and requires additional info. You can provide usage data for CSSO, to unlock reordering possibilities that is unsafe without this data. Take a look for the thread in csso-webpack-plugin to know how achieve it using webpack.
Hope that will help you.

@carlospnav
Copy link
Author

Correct me if I'm wrong (and it very well may be the case since I'm relatively new to web development) but what you are saying is that if the first media were to move down with the other queries, it would change the meaning of the code, since, if they applied to the same element, the media would take precedence over the other rule due to CSS cascading rules, correct? But isn't that intended behavior with media queries anyway? Do we not always want them taking precedence over whatever isn't inside a media query? If we author something in a media query to change some element but the general rule of that element always take precedence over the more specific media query, then there was no point having that query that in the first place, correct? Doesn't in the specific case of media queries, always moving them down(and thus having priority in the cascading ruling) with other queries, make sense?

@carlospnav
Copy link
Author

If this behavior, by default, might be too opinionated and take too much control out of developer hands (I mean, maybe they want to have a media query that does nothing?). Maybe a "optimizeMediaQueries" option parameter could work? (That is, of course, if what I said made sense).

@lahmatiy
Copy link
Member

But isn't that intended behavior with media queries anyway? Do we not always want them taking precedence over whatever isn't inside a media query?

That's may be correct in your case, but wrong in general. Possible problems are described in node-css-mqpacker: https://github.com/hail2u/node-css-mqpacker#notes

Maybe a "optimizeMediaQueries" option parameter could work?

Yep, I thought about such option. Not sure it's a good idea. At the same time, this can eliminate the need to use additional transformers like node-css-mqpacker.

We can turn this issue into a feature request to implement a new option for unsafe media moving.

lahmatiy added a commit that referenced this issue Aug 31, 2017
`mergeAtrule` goes first now
- lift `@keyframes` to the beginning of stylesheet (chunk), but after
charset and import rules
- remove duplicate (with the same name) keyframes (keep last one)
- new option `forceMediaMerge` to force media rules merging (#350)
@peterbe
Copy link
Contributor

peterbe commented Sep 7, 2017

With the new forceMediaMerge option, this can be closed, right?

@peterbe
Copy link
Contributor

peterbe commented Sep 7, 2017

Or perhaps we should wait till this feature is in a release.

@lahmatiy
Copy link
Member

lahmatiy commented Sep 7, 2017

Yep, I prefer to wait for the release.

@lahmatiy
Copy link
Member

@carlospnav v3.2.0 addresses the issue. Use forceMediaMerge option to achieve your goals.

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

No branches or pull requests

3 participants