upgrade to webpack v4 - Githubissues

thescientist13 commented 6 years ago

Related Issue

resolves #141

bonus: was able to resolve #139 as well.

Summary of Changes

Upgrade to webpack v4

Updated webpack, added webpack-cli, using contrib version of html-webpack-plugin, upgraded dependencies as needed
- Removed CommonsChunkPlugin
- Removed vendor.js and implemented optimization configuration
- Enabled development and production modes
- Enabled optimization configuration
- Was able to remove DefinePlugin
- swapped ETWP with mini-css-extract-plugin
Upgraded build to NodeJS v.8.9.4 (LTS) per recommendation from webpack team
Cleaned up some of the devDependencies

Stats

Before: 99.19s - https://circleci.com/gh/ProvidenceGeeks/website-frontend/299#build-timing/containers/0
Now: 35.07s - https://circleci.com/gh/ProvidenceGeeks/website-frontend/308#build-timing/containers/0

So could be potentially be a 50% savings even with another 15s of headroom. pretty nice! 🎆 🎉

But obviously without ExtractTextWebpackPlugin, our JS bundles are rather large

WARNING in asset size limit: The following asset(s) exceed the recommended size limit (244 KiB).
This can impact web performance.
Assets:
  vendor.49f48c8be89481dee508.bundle.js (478 KiB)
  index.5041108cb33b5dacaa6e.bundle.js (1.87 MiB)

WARNING in entrypoint size limit: The following entrypoint(s) combined asset size exceeds the recommended limit (244 KiB). This can impact web performance.
Entrypoints:
  index (1.87 MiB)
      index.5041108cb33b5dacaa6e.bundle.js
  vendor (478 KiB)
      vendor.49f48c8be89481dee508.bundle.js

WARNING in webpack performance recommendations:
You can limit the size of your bundles by using import() or require.ensure to lazy load some parts of your application.
For more info visit https://webpack.js.org/guides/code-splitting/

TODO

1. [x] Had to install tapable locally to appease fork of html-weboack-plugin. Update: resolved in webpack-contrib/html-webpack-plugin . Update: upstream has released a compat version
1. [x] Had to comment out eslint-loader, it's not supported with v4. Update: resolved in 2.0.0 release
1. [x] Had to disable ExtractTextWebpackPlugin, it is awaiting support for v4. Trying out MiniCssExtractPlugin - ran into this issue https://github.com/webpack-contrib/mini-css-extract-plugin/issues/50
1. [x] Had to disable FaviconsWebpackPlugin, it needs help getting support for v4
1. [x] Had to disable WebpackPwaManifest plugin, I opened an issue inquiring about support for v4 . Update: this issue seems to be tracking the release.
1. [x] Had to disable HtmlCriticalWebpackPlugin, likely because of ExtractTextWebpackPlugin but opened a PR to inquire about general webpack v4 support needed. Opened a PR to upgrade the plugin to webpack v4
1. [x] OptimizeCssAssetsPlugin left in but not sure if it's not running because ExtractTextWebpackPlugin is not enabled, but opened a PR to inquire about general webpack v4 support needed.

FDiskas commented 6 years ago

Error: Plugin could not be registered at 'html-webpack-plugin-before-html-processing'. Hook was not found.
BREAKING CHANGE: There need to exist a hook at 'this.hooks'. To create a compatiblity layer for this hook, hook into 'this._pluginCompat'.
    at Compilation.plugin (/***/node_modules/tapable/lib/Tapable.js:63:9)
    at Compilation.deprecated [as plugin] (internal/util.js:52:15)
    at /***/node_modules/webpack-pwa-manifest/dist/index.js:55:21

thescientist13 commented 6 years ago

Hey @FDiskas , just a heads up I think you might have meant to comment in a different PR (for webpack-contrib/html-webpack-plugin)? Or if you did mean this one, could you please elaborate? Just keep in mind this PR for upgrading our application is still heavily WIP (work in progress) though I have been documenting it extensively so as to provide a guide to others doing the same.

Hope that helps you out ✌️

FDiskas commented 6 years ago

@thescientist13 yes you are right. Sorry :v: :

thescientist13 commented 6 years ago

no worries @FDiskas , hope you're able to get your issued resolved ✅

thescientist13 commented 6 years ago

Overview

Final stats, all running locally (so no gzip, HTTP/2 etc) for getting a general feel of what things look like before / after the upgrade.

Before

Build Time: 44.71s

     2 assets
       4 modules
✨  Done in 44.71s.

Network Requests: 17 (3 JS, 2 CSS)

Downloaded: 1.3 MB (JS = 716 KB, CSS = 71.4 KB)

Finished: 918ms

Lighthouse score: 47 / 55 / 76 / 88 / 89

After

Build Time: 29.08s

                                  Asset      Size  Chunks             Chunk Names
    bbf1e3879689c911d25a8ebddb4d8f76.otf  13.9 KiB          [emitted]
    bc517149950afc2999fdec6f2e1bf064.otf  14.3 KiB          [emitted]
    Entrypoint mini-css-extract-plugin = *
       5 modules
✨  Done in 29.08s.

Network Requests: 17 (2 JS, 2 CSS)

Downloaded: 1.2MB (JS == 622 KB, CSS == 71.4 KB)

Finished: 534ms

Lighthouse score: 52 / 55 / 76 / 88 / 89

Summary

All metrics improved through this upgrade in particular build time just running locally, deploying this in the wild should bring even more improvements !! 🎉

Build Time: down 15.63s (34%!)

Network Requests: -

Downloaded: 0.1MB (13%, shaved about 100 KB in JavaScript!)

Finished: 384ms (41%)

Lighthouse score: 5 in performance (9.4%)

What's really compelling is although the network request size stayed the same, much less code was shipped down to the user which can seem to be correlated to the increase in Lighthouse score boost in the performance rating.

webpack is really packing that JavaScript 🏆

I think this change plus lazy loading the blog posts details route will really make big improvements to performance

@DevLab2425 mind taking a look?

thescientist13 commented 6 years ago

Stage Before

DevLab2425 commented 6 years ago

@thescientist13 Just finding the time to dig in, and first let me say I'm no Webpack expert, nor a PWA expert, I just play one on ~TV~ a daily basis.

Looking at your prod build I see a few slight differences from mine, but the majority looks similar to mine. (Considering I used this repo as a Webpack reference, there's no surprise there.)

A few things I have, that you do not:

I'm using multiple entry files, you have a single. This gives me smaller assets in hopes of avoiding blocking scripts
I have a minified CSS file injected into index.html. You seem to still have the multiple CSS file issue

A few thing you have, that I do not:

The latest version (0.0.8) of favicons-webpack-plugin - thanks!
The latest version (4.0.0) of optimize-css-assets-webpack-plugin - thanks!

You're using both the FaviconsWebpackPlugin and WebpackPwaManifest to generate icons. This actually generates too many, IMO. I've configured the 2 to play nice together. My relevant options look like: WPM icons array:

 icons: [
    {
      src: path.resolve('./src/assets/img/favicon-32x32.png'),
      sizes: [40, 48, 58, 60, 72, 80, 87, 120, 152, 180, 167, 512],
      destination: path.join('./', 'assets', 'icons'),
      ios: true
    }, 
    {
      src: path.resolve('./src/assets/img/favicon-32x32.png'),
      size: 1024,
      destination: path.join('./', 'assets', 'icons'),
      ios: 'startup'
    }
  ]

FWP icons object:

prefix: 'assets/icons/',
icons: {
    android: true,
    appleIcon: false,
    appleStartup: false,
    coast: false,
    favicons: true,
    firefox: true,
    opengraph: true,
    twitter: true,
    yandex: false,
    windows: false
  }

Hope this helps.

thescientist13 commented 6 years ago

thanks for the set of eyes on this @DevLab2425 !! 👀 🏆

I'm using multiple entry files, you have a single. This gives me smaller assets in hopes of avoiding blocking scripts

This is intentional and is considered best practice now in webpack now that CommonsChunkPlugin is deprecated and there is the optimization configuration option (which I am using in webpack.config.prod.js).

In reviewing the linked gist from that tweet, I've actually removed the cacheGroup setting since vendor is already a default group.

My local benchmark testing seems to corroborate that this recommended approach leads to an overall improvement as I saw one less HTTP request for JavaScript and smaller bundle size, so I would recommend you give it a shot too and let me know what your results look like! ✌️

I have a minified CSS file injected into index.html. You seem to still have the multiple CSS file issue

Hmm, anyway you could share your config files with me? (maybe a 1:1 DM on slack)

You're using both the FaviconsWebpackPlugin and WebpackPwaManifest to generate icons.

Good call. Any thoughts on just dropping FaviconsWebpackPlugin altogether?

thescientist13 commented 6 years ago

@DevLab2425 I think I've got #139 resolved now. Current production config

const commonConfig = require('./webpack.config.common');
const FaviconsWebpackPlugin = require('favicons-webpack-plugin');
const HtmlCriticalPlugin = require('html-critical-webpack-plugin');
const MiniCssExtractPlugin = require('mini-css-extract-plugin');
const OptimizeCssAssetsPlugin = require('optimize-css-assets-webpack-plugin');
const path = require('path');
const WebpackPwaManifest = require('webpack-pwa-manifest');
const webpack = require('webpack');
const UglifyJsWebpackPlugin = require('uglifyjs-webpack-plugin');
const webpackMerge = require('webpack-merge');

module.exports = webpackMerge(commonConfig, {

  mode: 'production',

  optimization: {
    minimizer: [
      new UglifyJsWebpackPlugin(),
      new OptimizeCssAssetsPlugin({
        cssProcessorOptions: { discardComments: { removeAll: true } }
      })
    ],
    splitChunks: {
      chunks: 'all',
      cacheGroups: {
        styles: {
          name: 'styles',
          test: /\.css$/,
          chunks: 'all',
          enforce: true
        }
      }
    }
  },

  module: {   
    rules: [{   
      test: /\.(s*)css$/,
      use: [
        MiniCssExtractPlugin.loader,
        'css-loader', 
        'sass-loader'
      ]
    }]
  },

  plugins: [

    new FaviconsWebpackPlugin({
      logo: './components/bootstrap/images/pvd-geeks-logo.png',
      emitStats: true,
      prefix: 'icons/',
      statsFilename: 'icons/stats.json',
      inject: true,
      title: 'Providence Geeks',
      background: '#bcbfc2',
      icons: {
        android: true,
        appleIcon: false,
        appleStartup: false,
        coast: false,
        favicons: true,
        firefox: true,
        opengraph: true,
        twitter: true,
        yandex: false,
        windows: false
      }
    }),

    new WebpackPwaManifest({
      name: 'Providence Geeks',
      short_name: 'PVD Geeks', // eslint-disable-line camelcase
      start_url: '.', // eslint-disable-line camelcase
      inject: true,
      fingerprints: true,
      ios: true,
      background_color: '#bcbfc2', // eslint-disable-line camelcase
      theme_color: '#1a2930', // eslint-disable-line camelcase
      icons: [{
        src: path.resolve('./src/components/bootstrap/images/pvd-geeks-logo.png'),
        sizes: [40, 48, 58, 60, 72, 80, 87, 120, 152, 180, 167, 512],
        ios: true
      }, {
        src: path.resolve('./src/components/bootstrap/images/pvd-geeks-logo.png'),
        size: 1024,
        ios: 'startup'
      }]
    }),

    new MiniCssExtractPlugin({
      filename: '[name].[contenthash].css',
      chunkFilename: '[id].[contenthash].css'
    }),

    new HtmlCriticalPlugin({
      base: path.resolve(__dirname, 'build'),
      src: 'index.html',
      dest: 'index.html',
      inline: true
    }),

    new webpack.optimize.ModuleConcatenationPlugin()
  ]
});

ProvidenceGeeks / website-frontend

upgrade to webpack v4 #142

Related Issue

Summary of Changes

Stats

TODO

Overview

Before

Build Time: 44.71s

Network Requests: 17 (3 JS, 2 CSS)

Downloaded: 1.3 MB (JS = 716 KB, CSS = 71.4 KB)

Finished: 918ms

Lighthouse score: 47 / 55 / 76 / 88 / 89

After

Build Time: 29.08s

Network Requests: 17 (2 JS, 2 CSS)

Downloaded: 1.2MB (JS == 622 KB, CSS == 71.4 KB)

Finished: 534ms

Lighthouse score: 52 / 55 / 76 / 88 / 89

Summary

Build Time: down 15.63s (34%!)

Network Requests: -

Downloaded: 0.1MB (13%, shaved about 100 KB in JavaScript!)

Finished: 384ms (41%)

Lighthouse score: 5 in performance (9.4%)

Stage Before