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

feat: minify extracted css #3857

Merged
merged 4 commits into from
Sep 8, 2018
Merged

feat: minify extracted css #3857

merged 4 commits into from
Sep 8, 2018

Conversation

manniL
Copy link
Member

@manniL manniL commented Sep 7, 2018

When extractCSS is enabled, it'll spit out the extracted CSS "as is", means unminified. Let's change this!

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have added tests to cover my changes (if not applicable, please state why)
  • All new and existing tests passed.

@@ -147,6 +148,15 @@ export default class WebpackClientConfig extends WebpackBaseConfig {
)
}

if (!this.options.dev && this.options.build.extractCSS) {
config.plugins.push(
Copy link
Member

Choose a reason for hiding this comment

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

This can use another plugin which is suggested by sorka and be put into optimization.minifier https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/README.md#minimizing-for-production

Copy link
Member Author

Choose a reason for hiding this comment

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

Will look into it this afternoon ☺️

Copy link
Member

Choose a reason for hiding this comment

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

Agree with @clarkdo too. NMFR/optimize-css-assets-webpack-plugin would be a better idea.

Also, we are already using cssnano (via postcss) for production build:

https://github.com/nuxt/nuxt.js/blob/e9bb9e7236eb0c9643daee8e557e6e254a08db8e/lib/builder/webpack/utils/postcss.js#L47

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the feedback 👌 I'll swap it then.

Yes, we use cssnano, but as it seems it will be applied before extraction and won't minify extracted CSS therefore. You can run the test without my changes and see it failing (or check this file which was extracted and not minified (this repo)

Copy link
Member Author

Choose a reason for hiding this comment

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

@pi0 @clarkdo Using optimize-css-assets-webpack-plugin doesn't work ☹️ (see last commit)

Copy link
Member

@clarkdo clarkdo Sep 7, 2018

Choose a reason for hiding this comment

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

@manniL Should be this.options.build.extractCSS ?
And also maybe we can add extractCSS.minifier: true for enabling it or do you have any idea ? Because if we add the plugins here, user can't change the behavior even they use extend

Copy link
Member Author

@manniL manniL Sep 7, 2018

Choose a reason for hiding this comment

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

Dang, right! 🙈
It goes well now, thx.

People could still override the whole minimizer, couldn't they?
If not, I'd agree with the extra flag there ☺️

(The check is !this.options.dev && !config.optimization.minimizer, so if config.optimization.minimizer is defined before, it won't get overriden)

Copy link
Member

Choose a reason for hiding this comment

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

Yep, you’re right, I’m ok with that and this plugin will be removed when webpack5 gets released with default css minifier, so could you add a TODO?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! There you go ☺️

@manniL manniL force-pushed the feat-minify-extracted-css branch 2 times, most recently from 5a526ae to 6755e60 Compare September 7, 2018 18:52
@codecov-io
Copy link

codecov-io commented Sep 7, 2018

Codecov Report

Merging #3857 into dev will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #3857      +/-   ##
==========================================
+ Coverage   97.56%   97.57%   +0.01%     
==========================================
  Files          18       18              
  Lines        1191     1196       +5     
  Branches      327      328       +1     
==========================================
+ Hits         1162     1167       +5     
  Misses         28       28              
  Partials        1        1
Impacted Files Coverage Δ
lib/builder/webpack/client.js 96.87% <100%> (+0.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b74d537...a6e072c. Read the comment docs.

@pi0 pi0 merged commit f879925 into nuxt:dev Sep 8, 2018
@manniL manniL deleted the feat-minify-extracted-css branch September 8, 2018 21:10
@milestonebooks
Copy link

I just updated nuxt to v2 and now I've come across a css glitch that I'm presuming is traceable to the new minify feature.

What started out as

background: url("data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' width='200' height='200' viewBox='0 0 200 200'%3E%3Cstyle type='text/css'%3E .c1, .c2 %7B transform-origin: 100px 100px; animation: x 2s ease-out infinite; %7D .c2 %7B animation-delay:-1s; %7D @keyframes x %7B from %7B transform: scale%280%29; opacity:.5; %7D to %7B transform:scale%281.0%29; opacity:0; %7D %7D %3C/style%3E%3Ccircle class='c1' cx='100' cy='100' r='20' fill='black' /%3E%3Ccircle class='c2' cx='100' cy='100' r='20' fill='black' /%3E%3C/svg%3E") no-repeat center/cover

which is an animated circle, became

background: url("data:image/svg+xml;charset=utf-8,%3Csvg xmlns='http://www.w3.org/2000/svg' width='200' height='200'%3E%3Cstyle/%3E%3Ccircle cx='100' cy='100' r='20' style='transform-origin:100px 100px;animation:x 2s ease-out infinite'/%3E%3Ccircle cx='100' cy='100' r='20' style='animation-delay:-1s;transform-origin:100px 100px;animation:x 2s ease-out infinite'/%3E%3C/svg%3E") no-repeat 50%/cover

which does not correctly apply the intend styling.

How do I disable whatever conversion is being applied here?

@manniL
Copy link
Member Author

manniL commented Oct 16, 2018

@milestonebooks Please refer to #4018 and https://nuxtjs.org/api/configuration-build/#optimizecss

Setting build.optimizeCSS to false should be sufficient

@milestonebooks
Copy link

@manniL

Setting build.optimizeCSS to false should be sufficient

I've done that, but the embedded styling is still being gutted.

@manniL
Copy link
Member Author

manniL commented Oct 17, 2018

@milestonebooks optimizeCSS will only disable optimizing extracted CSS. If you don't extract your CSS, you have to set-up postcss settings.

@milestonebooks
Copy link

@manniL I have the following settings in nuxt.config.js

build: {
    extractCSS: true,
    optimizeCSS: false,
},

but no success. I'm new to postcss. If that's the way to fix this, can you give me some pointers?

@milestonebooks
Copy link

Okay, this combination seems to do the trick:

build: {
  extractCSS: true,
  optimizeCSS: false,
  postcss: {
    plugins: {
      cssnano: {
        preset: [
          'default', {
            svgo: false,
          }]
      },
    }
  }
},

Ref: build.postcss; cssnano presets

@manniL
Copy link
Member Author

manniL commented Oct 17, 2018

@milestonebooks Instead of inlining your SVGs you can also load them like components with https://github.com/Developmint/nuxt-svg-loader

@milestonebooks
Copy link

@manniL Thanks, that could come in handy, but I don't see that it works for using svgs as background images.

@manniL
Copy link
Member Author

manniL commented Oct 17, 2018

@milestonebooks Well, that's true 🙈

@lock
Copy link

lock bot commented Nov 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Nov 16, 2018
@danielroe danielroe added the 2.x label Jan 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants