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

Transition to webpack 5 #4982

Merged
merged 14 commits into from Jun 9, 2022
Merged

Conversation

TheTrio
Copy link
Contributor

@TheTrio TheTrio commented May 23, 2022

What this PR does

Upgrades webpack to version 5

Changes

1. Switch to relative paths for css assets

This is because absolute paths are now relative to the root of the project.

2. Using asset modules instead of url-loader

url-loader has been deprecated for Webpack 5 in favor of asset modules. There are multiple asset types and I have gone with the general asset type. From the documentation -

Now webpack will automatically choose between resource and inline by following a default condition: a file with size less than 8kb will be treated as a inline module type and resource module type otherwise.

3. Single manifest file

Before, two manifest files were being created - one for css and another for JS. I've combined them into one manifests.json file so that the manifest plugin is only executed once.

4. Replace webpack-rtl-plugin with rtlcss-webpack-plugin

webpack-rtl-plugin was last updated on Mar 15, 2019. Despite there being a PR which adds support for Webpack 5, it hasn't been merged for over 8 months. So its been replaced with rtlcss-webpack-plugin which looks to be maintained more actively.

5. Add CssMinimizerWebpackPlugin to minify css

For some reason,webpack-rtl-plugin also minified the css files - both the RTL and non RTL ones. Since we no longer use that plugin, we need to minify the css with css-minimizer-webpack-plugin

6. Remove ExcludeAssetsPlugin

This plugin was used to clean up the empty JS files which were being generated by Webpack. With v5, Webpack no longer generates these files, so there was no need to use this plugin.

7. Remove Parallel Webpack

Parallel Webpack was used to speed up the build process by spreading the workload across multiple processors. The repo for this project has been archived and it does not support webpack 5. Webpack doesn't have native support for parallelism, but that might change in the future.

8. Use webpack dev server by default

Earlier, this was only being used with yarn hot. I couldn't get yarn hot to work so it might have been broken for a while. Regardless, yarn start now uses webpack dev server. The files are written to the disk presently, but I will try to use the server directly. I haven't been been able to make that work so far. The switch to webpack dev server also means that the webpage reloads automatically on code changes. It also shows warnings and errors inside of the browser.

9. Fix regex for coverage

The regex which split main.js into multiple modules broke because webpack uses arrow functions instead of the function keyword. I've changed that so that yarn coverage still works as expected.

There are some other configuration changes but these are the major ones I could think of.

Performance

These numbers are obviously not scientific but the time taken is consistent from the testing I've done.

command cache Webpack 5 Webpack 4
yarn start no 70s 40s
yarn start yes 10s 10s
yarn build no 101s 75s
yarn build yes 41s 25s

For rebuilds, it takes about 2000ms the first time and the subsequent rebuilds are much faster, around 800ms. This is probably due to the source map configuration. Unfortunately, if we want the right line numbers in the error messages, I think this is the best that can be done. More details can be found here

It looks like webpack now generates arrow functions instead of using the
function keyword. This, along with a change in the source map
configuration, broke yarn coverage.

I have updated the regex and switched back to the old source map
configuration for the time being. This should hopefully suffice
This package hasn't been updated for webpack 5
The webpack-rtl-plugin hasn't been updated for 3 years and doesn't
support webpack 5. This switch also means that we have to minify the css
ourselves(which was earlier done by the rtl plugin) using css-minimizer-webpack-plugin
@TheTrio TheTrio marked this pull request as ready for review May 24, 2022 15:44
@ragesoss
Copy link
Member

Super helpful summary! Can you provide before/after numbers (ie, v4 vs v5) for the yarn commands?

@TheTrio
Copy link
Contributor Author

TheTrio commented May 24, 2022

Edited. I think the main reason for the performance difference is because of parallel webpack running things in, well, parallel.

@TheTrio
Copy link
Contributor Author

TheTrio commented May 24, 2022

Here are the new numbers

command cache Webpack 5 Webpack 4
yarn start no 20s 40s
yarn start yes 10s 10s
yarn build no 58s 75s
yarn build yes 41s 25s

It looks like ESLint is the culprit here. I've enabled lintDirtyModulesOnly which caches dirty modules only. So while the initial build times are faster, during development, when you edit the file for the first time, it will take time - 3-7 seconds. Subsequent edits will be faster and as the cache builds up, it will drop to around 1000ms.

Essentially what we were doing during the build process(building up the eslint cache) is now being done on demand - when you edit files. This slows initial edits during development.

Spawning threads during development slowed down the linting. So its now
enabled only during production builds. There's also a new command for
disabling linting altogether
@TheTrio
Copy link
Contributor Author

TheTrio commented May 25, 2022

Since most editors and IDEs run ESlint in the background anyways, I've added a new command - start:no-lint. This disables the ESlint plugin - which means that even the first edit is compiled instantly.

yarn start is slow on the first edit, but after that(as the cache builds up), it should be almost as fast as yarn start:no-lint

css.webpack.js Outdated
}
]
},
plugins: [
// extracts CSS to a separate file
new MiniCssExtractPlugin({
filename: env.development ? '../stylesheets/[name].css' : '../stylesheets/[name].[hash].css',
filename: env.development ? '../stylesheets/[name].css' : '../stylesheets/[name].[contenthash].css',

Choose a reason for hiding this comment

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

What other hash options are available?

Copy link
Contributor Author

@TheTrio TheTrio Jun 7, 2022

Choose a reason for hiding this comment

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

Looks like the supported substitutions are hash, fullhash, contenthash and chunkhash. The reason I went with contenthash was because the webpack 5 migration guide recommended it

// node environment
new webpack.DefinePlugin({
'process.env': {
NODE_ENV: JSON.stringify(mode),

Choose a reason for hiding this comment

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

Is this available in webpack 5?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is but it looks like since Webpack 4 defining mode should set process.env.NODE_ENV automatically - so this might've been redundant as well.

css.webpack.js Outdated
filename: '../stylesheets/rtl-[name].[contenthash].css'
// this is done only in production/coverage
...(env.production || env.coverage ? [new RtlCssPlugin({
filename: '../stylesheets/rtl-[name].[fullhash].css'

Choose a reason for hiding this comment

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

What other hash options are available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rtl-css-plugin still has some deprecation warnings when using contenthash. Which is why I used fullhash here.

output.devServer = {
devMiddleware: {
publicPath: path.join(__dirname, '/public'),
writeToDisk: true,

Choose a reason for hiding this comment

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

Why don't we keep this in memory itself? Why do we need to write to disk?

Choose a reason for hiding this comment

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

currently has an issue with dynamic import bundles if we switch to in-memory

js.webpack.js Outdated
optimization: {
splitChunks: {
cacheGroups: {
vendors: {
defaultVendors: {

Choose a reason for hiding this comment

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

Can we explore doing splitting for vendor bundle?

css.webpack.js Outdated
optimization: {
minimizer: [
'...',
new CssMinimizerPlugin(),

Choose a reason for hiding this comment

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

Explore if we can pass any options that might be helpful.

Choose a reason for hiding this comment

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

@TheTrio Were you able to explore this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The defaults were good enough in my opinion so there wasn't much to configure. As for CSS Nano options, auto prefixing was the only one that felt necessary to enable but then I found out that stylus loader already does that.

So no, not much there I think. Do you have some specific configuration in mind?

@@ -23,39 +22,41 @@ const entry = {
};

module.exports = (env) => {
const doHot = env.development && !env.watch_js;

Choose a reason for hiding this comment

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

Need to check how this works with Ruby server and enable hot module replacement.

js.webpack.js Outdated
const LodashModuleReplacementPlugin = require('lodash-webpack-plugin');
const ESLintPlugin = require('eslint-webpack-plugin');
const MomentLocalesPlugin = require('moment-locales-webpack-plugin');
const webpack = require('webpack');

const jsSource = `./${config.sourcePath}/${config.jsDirectory}`;
const appRoot = path.resolve('./');

Choose a reason for hiding this comment

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

Remove this.

js.webpack.js Outdated
? path.resolve(appRoot, `${config.outputPath}/${config.jsDirectory}`)
: path.resolve(`${config.outputPath}/${config.jsDirectory}`);
const devtool = env.coverage ? 'cheap-module-source-map' : 'cheap-eval-source-map';
const outputPath = path.resolve(appRoot, `${config.outputPath}/${config.jsDirectory}`);

Choose a reason for hiding this comment

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

Suggested change
const outputPath = path.resolve(appRoot, `${config.outputPath}/${config.jsDirectory}`);
const outputPath = path.resolve(`${config.outputPath}/${config.jsDirectory}`);

js.webpack.js Outdated
const devtool = env.coverage ? 'cheap-module-source-map' : 'cheap-eval-source-map';
const outputPath = path.resolve(appRoot, `${config.outputPath}/${config.jsDirectory}`);

let devtool;

Choose a reason for hiding this comment

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

Suggested change
let devtool;
let devtool = 'eval-cheap-source-map';

js.webpack.js Outdated
optimization: {
splitChunks: {
cacheGroups: {
vendors: {
defaultVendors: {

Choose a reason for hiding this comment

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

Can explore later on how to split vendor checks and make it work with Ruby

@@ -14,7 +14,7 @@ namespace :generate do
funs.each do |fun|
counter += 1
text = text.sub(fun, "*/module_#{counter}") # Replaces the function with module_{number}
fun = fun.gsub(%r/\/[*]{3}\/ \(function/, "var module_#{counter} = (function")
fun = fun.gsub(%r/\/[*]{3}\/ \(/, "var module_#{counter} = (")

Choose a reason for hiding this comment

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

@ragesoss Need to validate if coverage.rake file is working as expected since the output in public/assets/javascripts/main.js might have changed from webpack 4 to 5.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I tried it out locally and the coverage.rake is still working.

@ragesoss
Copy link
Member

ragesoss commented Jun 7, 2022

@TheTrio @dhruvdutt let me know when both of you are happy with this PR. I can merge it as soon as you both think it's ready.

@dhruvdutt
Copy link

Let's try to get rid of js.webpack and css.webpack and keep only webpack.js and probably a different file for webpack plugins if it's too large to put in the same file.
Keeping js and css files differently is an anti-pattern and the biggest problem is conflicts while merging both these objects. The earlier version had overrides, conflicting rules, objects and variables, all of which make it difficult to visualize the final config that's going to get applied.
IMO having one large file organized and documented inline is better than different files which merge with conflicts and ambiguous override states.

@TheTrio
Copy link
Contributor Author

TheTrio commented Jun 7, 2022

Yeah having just one webpack config would definitely make things simpler. I'll get that done by tomorrow.

Apart from that, I don't think there's anything major left to do in this PR. There's still some stuff I want to explore - like HMR, speeding up eslint and switching to memory mode for the dev server.

But I think those can be tracked independently of this PR.

@dhruvdutt
Copy link

@TheTrio Can you please create issues for things we're going to be looking into separately?

  • HMR
  • eslint optimizations
  • Webpack dev server in-memory chunks
  • Vendor chunking

@ragesoss ragesoss merged commit 485ef7a into WikiEducationFoundation:master Jun 9, 2022
@TheTrio TheTrio deleted the develop branch June 9, 2022 15:34
@TheTrio TheTrio mentioned this pull request Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants