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

Enable CSS Hot Module Replacement and optimize css output for production #5813

Merged
merged 4 commits into from Sep 5, 2018

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Sep 4, 2018

Enable HMR for CSS

Current webpack settings uses mini-css-extract-plugin for both development and production. This plugin extract css into many small files based on the modules that imports them.

However, per the package's README, it is recommended for production only especially if we want Hot Module Replacement (HMR) for CSS. To use HMR, we need to use style-loader when in development mode. Otherwise, the page will not update after saving CSS files.

https://github.com/webpack-contrib/mini-css-extract-plugin#advanced-configuration-example

To ensure that style-loader works correctly, I also need to modify the html templates to import theme js entry points. Because style-loader injects css code via js that writes <style> tags instead of exporting css files.

Optimize CSS output

Current CSS output from webpack are not minified. This PR adds optimize-css-assets-webpack-plugin to minify CSS outputs. The minification reduce CSS size by ~100kb

pasted_image_9_4_18__11_33_am

Testing

I have tested building in production and running dev server. Both are working fine.

@graceguo-supercat @xtinec @hughhhh @conglei @williaster

@mistercrunch
Copy link
Member

@xtinec said she knows Webpack very well and can help review & advise here

@@ -2,7 +2,9 @@ const path = require('path');
const webpack = require('webpack');
const CleanWebpackPlugin = require('clean-webpack-plugin');
const MiniCssExtractPlugin = require('mini-css-extract-plugin');
const OptimizeCSSAssetsPlugin = require("optimize-css-assets-webpack-plugin");
Copy link
Contributor

Choose a reason for hiding this comment

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

[Nit] '' here to get past the linter.

@xtinec
Copy link
Contributor

xtinec commented Sep 4, 2018

Looks good aside from the small thing to use ' in place of ". 🎉

@kristw
Copy link
Contributor Author

kristw commented Sep 4, 2018

Thanks @mistercrunch , @xtinec

@codecov-io
Copy link

codecov-io commented Sep 4, 2018

Codecov Report

Merging #5813 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5813   +/-   ##
=======================================
  Coverage   63.82%   63.82%           
=======================================
  Files         364      364           
  Lines       23099    23099           
  Branches     2587     2587           
=======================================
  Hits        14744    14744           
  Misses       8340     8340           
  Partials       15       15

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 eb41756...1232eaf. Read the comment docs.

Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

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

LGTM! looks like I had the 'mini-css-extract-plugin backwards 🤦‍♂️ 🙃 .

thanks for making the dev env 💯x better!

@williaster williaster merged commit 0c33f80 into apache:master Sep 5, 2018
@kristw kristw deleted the kristw-css branch September 5, 2018 18:52
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Oct 12, 2018
…ion (apache#5813)

* Enable HMR for css. Use style-loader for development

* optimize css output

* remove mini-css-extract when dev

* change double-quote to single-quote
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
…ion (apache#5813)

* Enable HMR for css. Use style-loader for development

* optimize css output

* remove mini-css-extract when dev

* change double-quote to single-quote
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants