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

Migrate to Webpack 5 and improve bundle size #546

Merged
merged 17 commits into from
Jan 19, 2021

Conversation

LordSputnik
Copy link
Member

Problem

bookbrainz-site was dependent on webpack 4, and the bundle was 809 kB in size (compressed). Thought that I might be able to achieve some speed and build size improvements by switching to the new webpack 5. Additionally, there was a problem with the way assets were being resolved in the less pipeline (https://tickets.metabrainz.org/projects/BB/issues/BB-573).

Solution

Switches to webpack 5, and update all of the webpack loaders. Updating less-loader and switching to webpack module resolution fixes BB-573. Also cut out some unneeded dependencies (e.g. style-loader), and followed the webpack migration steps for v4->v5 (https://webpack.js.org/migrate/5/), which cleaned up some deprecated features.

Once I was on webpack 5, I saw quite a bit of slow-down in the build process, and not much improvement in build size. So I looked into why tree-shaking wasn't working, and fixed that, then I updated the way we reference FontAwesome icons to allow them to be tree-shaken. I also removed moment.js, which was only used by Chart.js, and replaced it with date-fns, which was already in the bundle. As a result, bundle size has been reduced by about 60% (now ~350 kB compressed). Further size gains could be achieved by taking Chart.js out of the bundle (it's only used on one page, and 50 kB of the bundle), but that needs a bit of work to use dynamic importing. Also, lodash and immutable are taking up a bit more space than I would expect.

In terms of speed improvements, speed initially doubled going from v4 to v5, from 22 s to about 50 s. I analyzed the build speed using https://github.com/stephencookdev/speed-measure-webpack-plugin and found that ESLintPlugin was taking a long time to run, so I added a rule so that it only gets called for modules in "src/". This reduced build time back down to about 22 s.

Areas of Impact

Mostly impacts the webpack config. FontAwesome elements in React pages have also been updated to use icon objects directly rather than string references to icons. The babel config has changed slightly. The main style.less file has had some imports updated. I've tested production compilation, the editor profile page (where the Chart.js chart is), server-side rendering and hot module replacement - all seem to work OK.

Copy link
Contributor

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Nicely done, another good critical update. Thank you !
And what can I say about a 60% bundle size reduction other than "woohoo"‽

The image files you moved out of the static folder are being automatically copied back into static/images/ by webpack, leaving my git directory dirty.
That doesn't look intentional; I guarantee some contributors will commit those files by mistake. I'm not exactly sure what is the best option here: have it committed twice?

There are still some oddities with the build system we'll eventually want to improve, but none that were introduced in this PR.

src/server/app.js Outdated Show resolved Hide resolved
webpack.client.js Outdated Show resolved Hide resolved
Comment on lines +110 to +108
chunks: 'all',
name: 'bundle'
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this is not equivalent to the previous configuration, but I'd have to dig into how to properly optimize before I could say there's an issue.
I found everything working both in dev and prod, so there is no obvious breaking issue at the very least.

@MonkeyDo
Copy link
Contributor

There's also an ESLint issue:

/home/travis/build/bookbrainz/bookbrainz-site/src/client/entity-editor/common/validation-label.tsx
  22:1   error  Imports should be sorted alphabetically                                             sort-imports
  22:9   error  IconDefinition not found in '@fortawesome/react-fontawesome'                        import/named
  22:25  error  Member 'FontAwesomeIcon' of the import declaration should be sorted alphabetically  sort-imports

@coveralls
Copy link

coveralls commented Jan 16, 2021

Coverage Status

Coverage increased (+0.2%) to 61.049% when pulling 3efc45f on LordSputnik:webpack5 into 2e762c9 on bookbrainz:master.

Also change location of Bookshelf.jpg image, since this
is referenced directly in less file, and it's simpler to
reference a path relative to the less file and let webpack
handle things than to work around webpack errors when it can't
be resolved.
Since we serve up 'static/<path>' on '/<path>', publicPath
should point to '/'.
This had been added to webpack-dev-middleware and can be used
to replace WriteAssetsWebpackPlugin.
Also necessary to set math to be always evaluated, since the
default changed between less 3 and less 4, to only evaluate
math within parentheses.
No side-effects from these:

babel-loader               :  ^8.1.0 ->  ^8.2.2
compression-webpack-plugin :  ^6.1.0 ->  ^6.1.1
eslint-webpack-plugin      :  ^2.3.0 ->  ^2.4.1
file-loader                :  ^2.0.0 ->  ^6.2.0
react-hot-loader           :  ^4.3.4 -> ^4.13.0
webpack-hot-middleware     : ^2.22.3 -> ^2.25.0
This was deprecated in webpack 4 - see "Update outdated options" in:
https://webpack.js.org/migrate/5/
* update webpack from ^4.44.2 to ^5.11.1
* add 'fullySpecified: false' workaround for missing file extensions in
  runtime-corejs2/helpers/esm
* replace images file-loader with WebPack 5 Asset Module
* fix error causes by MiniCssExtractPlugin and update CSS file
references
* remove optimization.moduleIds as default is now sufficient
* restore defaults for splitChunks, which are equivalent to the
  current config
* further restrict use of babel-loader with 'include' rather than
  'exclude' rule
* use default naming for HMR chunk output - webpack docs advise this
CommonJS is only needed when executing files in node.js,
while webpack works better with ES6 modules. babel-env will
take care of converting CommonJS where needed, using 'caller'
data.

https://babeljs.io/docs/en/babel-preset-env#modules
This is simpler, avoids the need to maintain a separate list
of 'used' FontAwesome icons and is easier for Webpack to
optimize. Also allows for static type checking, whereas strings
are liable to point to unresolved icons, which isn't detected
automatically.
* only run ESLintPlugin on files within src - previously, the plugin
  was invoked for all files but wasn't configured to check anything
  outside src, which wasted time.
date-fns is already used our client bundle, whereas moment is only
used by Chart.js.
Copy link
Contributor

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

I'll follow up by updating #544 after merging this.

@MonkeyDo MonkeyDo merged commit 95146c7 into metabrainz:master Jan 19, 2021
@LordSputnik LordSputnik deleted the webpack5 branch January 20, 2021 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants