department-of-veterans-affairs / va.gov-team

Public resources for building on and in support of VA.gov. Visit complete Knowledge Hub:
https://depo-platform-documentation.scrollhelp.site/index.html
283 stars 204 forks source link

Upgrade to Webpack 5 #11510

Closed rianfowler closed 2 years ago

rianfowler commented 4 years ago

Overview

billfienberg commented 4 years ago

Related/duplicate https://github.com/department-of-veterans-affairs/va.gov-team/issues/14718

meganhkelley commented 3 years ago

Notes from Chris: Test build locally in all environments (success). All 12 reviews received and have answered all their questions Further testing in IE uncovered an issue with the search dropdown (doesn’t appear on click) Also timed the builds locally, found that webpack 5 is slower than 4. Wonder if this can be improved

Need to — Determine cause of IE search dropdown issue and fix. Perform additional testing if time allows

alexandec commented 3 years ago

Resuming work on this ticket. Benchmarks for "yarn build:webpack" (averages of 5 runs each):

So Webpack 5 takes about twice as long to complete a build. I'll look into whether there are configuration changes to reduce the Webpack 5 build time.

meganhkelley commented 3 years ago

Will this add literally 23.3 seconds to the build? Or is that magnified/compounded in some way for a greater effect? If it's just an extra 23 seconds, I would assume it's a pretty low bar for the question "are the improvements worth the extra time"?

alexandec commented 3 years ago

Since Webpack runs twice on Jenkins (in the build stage and the prearchive stage), that number would be doubled. But actually, those numbers above were for the local development build. I'll run the benchmarks again using the production build, which requires more processing power. That will give us a more realistic number since the staging and production builds on Jenkins both do a production build.

alexandec commented 3 years ago

Build times for production build on Jenkins (averages):

Difference is 11.18 - 5.59 = 5.59 minutes. Since we run the build twice on Jenkins, that's 5.59 * 2 = 11.18 minutes total added to the build time. I'll profile the build to see where that time is going and what we can do to reduce it.

alexandec commented 3 years ago

Results of "yarn build:webpack --env buildtype=vagovprod" with speed-measure-webpack-plugin (SMP) installed. TLDR: when it comes to plugins and loaders, webpack 4 and 5 report similar timing. So the multi-minute difference in total build time is not caused by plugins or loaders. There must be something going on inside webpack itself.

Note that the comparison isn't perfect because I did have to disable TerserPlugin, MiniCssExtractPlugin, WebpackManifestPlugin, and and SourceMapDevToolPlugin for the webpack 5 run (SMP didn't play well with those). But the results are still useful even so.

WEBPACK 4 ✔ Webpack Compiled successfully in 48.47s

SMP ⏱
General output time took 48.47 secs

SMP ⏱ Plugins IgnorePlugin took 9.36 secs WebpackBarPlugin took 3.48 secs SourceMapDevToolPlugin took 2.039 secs ManifestPlugin took 0.328 secs MiniCssExtractPlugin took 0.026 secs DefinePlugin took 0.008 secs

SMP ⏱ Loaders mini-css-extract-plugin, and css-loader, and postcss-loader, and sass-loader took 38.77 secs module count = 64 css-loader, and postcss-loader, and sass-loader took 38.71 secs module count = 64 babel-loader took 31.57 secs module count = 2287 modules with no loaders took 22.18 secs module count = 1782 svg-url-loader took 0.854 secs module count = 89 url-loader took 0.756 secs module count = 118 file-loader took 0.047 secs module count = 48 css-loader took 0.016 secs module count = 1 null-loader took 0.005 secs module count = 19

WEBPACK 5 ✔ Webpack Compiled successfully in 3.92m

SMP ⏱
General output time took 3 mins, 55.11 secs

SMP ⏱ Plugins IgnorePlugin took 5.15 secs WebpackBarPlugin took 0.867 secs DefinePlugin took 0.001 secs

SMP ⏱ Loaders css-loader, and postcss-loader, and sass-loader took 30.76 secs module count = 64 babel-loader took 21.25 secs module count = 2280 modules with no loaders took 11.99 secs module count = 1590 svg-url-loader took 0.51 secs module count = 29 url-loader took 0.485 secs module count = 49 css-loader took 0.246 secs module count = 1 file-loader took 0.155 secs module count = 24 null-loader took 0.005 secs module count = 19

alexandec commented 3 years ago

Tested builds with and without sourcemaps:

Webpack 4 prod with sourcemap: 347s Webpack 4 prod without sourcemap: 215s Webpack 4 prod without sourcemap + terser: 47s

Webpack 5 prod with sourcemap: 645s Webpack 5 prod without sourcemap: 312s Webpack 5 prod without sourcemap + terser: 295s

One finding here is that if we didn't use sourcemaps in the production build (or we used lower-accuracy sourcemaps which are quicker to build), we could save a significant amount of build time. But if we want to stick with the current high-cost high-quality sourcemaps, Webpack 5 is very slow to generate them. I haven't been able to determine why.

alexandec commented 3 years ago

Some other things I tried:

meganhkelley commented 3 years ago

Putting this on hold due to Webpack 5 increasing build time. Will return to this as a possibility after app isolation to see if the impact has lessened.

alexandec commented 3 years ago

Build directory file sizes and file counts generated with rm -rf build/vagovprod, yarn build:webpack:prod, du -ch build/vagovprod/generated/*.js:

So webpack 5 is generating about twice as much JS code as it should be even though the number of JS files generated is the same. Don't know the cause yet, but it's a good area to investigate.

meganhkelley commented 3 years ago

Picking this up again as part of dependency upgrade work

jhonnyoddball commented 3 years ago

Webpack related dependencies

jhonnyoddball commented 3 years ago

Upgrading notes

alexandec commented 2 years ago

Tasks completed and merged:

Tasks completed in new webpack-5-upgrade branch:

Tasks remaining:

alexandec commented 2 years ago

More details on the font/image issue: it is caused by setting url: false in the css-loader config. That setting causes css-loader to ignore url() imports in scss files like:

@font-face {
  font-family: "Bitter";
  font-style: normal;
  font-weight: 400;
  src:
    local("Bitter Regular"),
    local("Bitter-Regular"),
    url(/fonts/bitter-regular.woff2) format("woff2"),
    url(/fonts/bitter-regular.ttf) format("truetype");
  unicode-range: U+0000-00FF, U+0131, U+0152-0153, U+02C6, U+02DA, U+02DC, U+2000-206F, U+2074, U+20AC, U+2212, U+2215;
}

Ignoring this import means webpack does not place the font file in the generated directory. Later, we serve the page to the browser and it sees a tag like:

<link rel="preload" href="/generated/bitter-bold.woff2" as="font" type="font/woff2" crossorigin>

The browser requests the font file but it is not present, and the request fails.

The most obvious solution is to change the css-loader config to url: true. However this results in the webpack build hanging and never completing. We need to determine the cause of the hang and fix it, or find an alternate approach to handing url() loading of fonts and images.

alexandec commented 2 years ago

I found a solution for the font/image issue. It was caused by a breaking change in css-loader. The change is documented in the css-loader changelog, and appears in version 4.0.0:

url() resolving algorithm now handles absolute paths instead of ignoring them. This can break builds which relied on absolute paths to refer to the asset directory. (bc19ddd)

This is exactly what were were relying on, for example with url(/fonts/bitter-regular.woff2). We expect that to map to src/site/assets/fonts/bitter-regular.woff2, and prior to version 4.0.0, it did. But with 4.0.0 it maps to fonts/bitter-regular.woff2 in the top level of the repo instead, which doesn't exist.

Instead of upgrading css-loader from 1.0.0 all the way to 6.5.1, we can upgrade to 3.6.0 instead. This is prior to the breaking change, but still works with webpack 5 and related upgrades. This will unblock the webpack upgrade. In the future we can address the breaking change and upgrade css-loader all the way to the latest version.

alexandec commented 2 years ago

We had an additional CSS issue, which is that certain CSS filenames were replaced with numbers with webpack 5:

Only in webpack-5-upgrade/build/vagovprod/generated: 2841.css
Only in webpack-5-upgrade/build/vagovprod/generated: 8832.css
Only in webpack-5-upgrade/build/vagovprod/generated: 8859.css
Only in webpack-5-upgrade/build/vagovprod/generated: 9505.css
Only in vets-website/build/vagovprod/generated: disability-rating-calculator.css
Only in vets-website/build/vagovprod/generated: find-va-forms.css
Only in vets-website/build/vagovprod/generated: resources-and-support-search.css
Only in vets-website/build/vagovprod/generated: third-party-app-directory.css

I found a custom MiniCssExtractPlugin function which was no longer needed, and removed it. This reversion to the default behavior fixed the problematic filenames.

timwright12 commented 2 years ago

This was released to staging and dev today with commit: https://github.com/department-of-veterans-affairs/vets-website/commit/6e0ef66393bd6ac9f5dd012e045257e055fc3471

timwright12 commented 2 years ago

This has been deployed to production