wp-media / wp-rocket

Performance optimization plugin for WordPress
https://wp-rocket.me
GNU General Public License v2.0
698 stars 217 forks source link

Get rid of "Combine CSS files" option #5909

Closed piotrbak closed 1 year ago

piotrbak commented 1 year ago

Is your feature request related to a problem? Please describe. HTTP/1 is outdated protocol and CSS Combination is only useful in this specific circumstance for performance purposes. It's also good in terms of browser caching, as the single files will be saved and shared between all pages.

Describe the solution you'd like The further details related to this improvement are available here.

CrochetFeve0251 commented 1 year ago

Scope a solution

The first step will be to get rid of the actual settings section. For that we will have to remove inside Page.php from the line 682 to 705.

We will have to remove the logic inside Minify CSS:

if ( $this->options->get( 'minify_css' ) && $this->options->get( 'minify_concatenate_css' ) ) {
 $this->set_processor_type( new Combine( $this->options, $assets_local_cache ) );
} elseif ( $this->options->get( 'minify_css' ) && ! $this->options->get( 'minify_concatenate_css' ) ) {
 $this->set_processor_type( new Minify( $this->options, $assets_local_cache ) );
}

to :

 if ( $this->options->get( 'minify_css' ) ) {
 $this->set_processor_type( new Minify( $this->options, $assets_local_cache ) );
}

Then we will have to remove everywhere this code is used in the core logic:

Then we will have to remove it inside 3rd parties:

To clean assets from wpr on update, we will clear the cache.

Finally we will have to make sure all tests pass.

Estimate effort

Effort M

rorytassell commented 1 year ago

The combine CSS feature actually helps with SEO.

Allowing GoogleBot to crawl less CSS/JS files is ALWAYS a good thing. While minifying files is good for performance, you still have a similar amount of individual CSS files (i.e. http requests) which GoogleBot (or bing etc) will need to crawl, thus using precious crawl budget.

Not sure who signed this off, or why it got worked/released so quickly considering there are plenty of other bug fixes and features missing.

scottstanford commented 1 year ago

Agreed with @rorydesign - the combine CSS feature is essential (and ultimately could be disabled in the UI if someone doesn't want to use it). Our site structure uses a dynamic set of small css files, but combining them allowed for much better performance and page load times.

With this feature removed, we are getting much longer page loads and have had to rollback to a previous version.

DahmaniAdame commented 1 year ago

Many factors motivated the change:

With this feature removed, we are getting much longer page loads and have had to rollback to a previous version.

@scottstanford would you mind sending me the audit results from https://www.webpagetest.org/ to adame@wp-media.me for combined vs. no combined?

I'll be curious to see the impact on your specific use case.

While minifying files is good for performance, you still have a similar amount of individual CSS files (i.e. http requests) which GoogleBot (or bing etc) will need to crawl, thus using precious crawl budget.

@rorydesign using Remove Unused CSS will have a similar effect.

We understand that this change won't be to the taste of some of our users. We apologize for the inconvenience, but it was a necessary step to take on our side.

There is room for reconsideration if there is a drastic penalty for the vast majority of our users.

aharonyltd commented 1 year ago

Bad decision by wp rocket team Some sites load CSS files one after the other and cause CLS issues Not in every site we can use the CSS removal feature because it still doesn't work smoothly.

I improve the performance of websites, I won the first place in the competition to improve the speeds of Cloudways WooCommerce websites

I suggest you bring back the Combine CSS feature

There are sites where I need to install another plugin that will do this action

Secondly, you did something that sometimes things don't work well with the CSS There are such paths that are added: https://wordpress-505400-1873104.cloudwaysapps.com/wp-content/cache/background-css/wordpress-505400-1873104.cloudwaysapps.com/wp-content/cache/min/1/wp-content/plugins/ shortcodes-ultimate/includes/css/shortcodes.css?ver=1694525856&wpr_t=1694525856

This also caused the broken loading of some icons and fonts on the sites that I am improving the speed of

vmanthos commented 1 year ago

This also caused the broken loading of some icons and fonts on the sites

@piotrbak FYI, I checked this :point_up_2: with lazyload for CSS background images and it's working fine. Icons are also displayed so this might be coming from Remove Unused CSS if that's enabled.

piotrbak commented 1 year ago

Thank you for the feedback, it's very apprieciated. Please remember that we use GitHub for development purposes only.

If you experience any performance decrease because of the lack of CSS Combination feature, please reach our Support Team with real data or example, we'll be more than happy to analyze that and find a proper solution. Each decision made is preceded by research and benchmarks.

@aharonyltd I'm sorry to hear you are experiencing problem with new feature. Please reach our Support Team, we'll investigate the problem and fix it very soon.