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

Attempt #3: upgrade to Webpack v5 #22580

Merged
merged 42 commits into from
Jul 19, 2021
Merged

Attempt #3: upgrade to Webpack v5 #22580

merged 42 commits into from
Jul 19, 2021

Conversation

valerybugakov
Copy link
Member

@valerybugakov valerybugakov commented Jul 5, 2021

Context

I was looking into DLL Plugin integration into the web app and, because of some Webpack bug, tried out Webpack 5 first.
Here we are.

This work is based on existing PRs: #14597 and #21061. Thanks, @felixfbecker, and @eseliger for looking into this upgrade.
Hopefully, we can nail it this time.

To debug the out-of-memory issue, I've upgraded each package at a time individually per commit.
Good news: all checks are green!

Changes

  • Webpack and all loaders are upgraded to the latest versions.
  • Webpack-dev-server is upgraded to 4.0.0-beta.3
  • Web app, browser extension, and Storybook Webpack configs are updated for Webpack 5.
  • Filesystem cache is enabled in the development environment in all Webpack configs.
  • React-hot-loader is removed in favor of the react-refresh-webpack-plugin.

React Fast Refresh

React hot loader, that we currently use, is deprecated, and it didn't work out of the box with Webpack 5 upgrade. Because hot-reload is essential for the frontend development experience, this PR includes migration to the react-refresh-webpack-plugin. It was impossible to split this work into separate PRs because Webpack 5 and react-refresh-plugin depend on each other's latest versions.

Here's a couple of great resources to read about fast-refresh. Also, the plugin contains an important doc about limitations which we should be aware of to write fast-refreshable React code.

Pinned Webpack 5

A small explainer on why it's needed:

  • It's required to have consistent Webpack types. Webpack 5 provides its own typings, but some packages still use @types/webpack, which results in type conflicts.
  • Storybook installs Webpack 4 by default as a dependency which results in multiple warnings during the development process. At the same time, Webpack 4 is not used at all because preview and manager builders are using v5 now.

We're not pinning a specific version of the Webpack, so it should be ok till the issues above are solved by maintainers. Leave a comment if you have a better solution in mind.

Stats

Approximate timings based on local testing of the hot start commands — development mode + cache is already in place.

Workspace Start Before Start After
Web app 30s 6.5s
Browser extension 26s 3s
Storybook 30s 7.5s or 3.5s with DLL Plugin

Web app production bundle:

Before After
image image

Known issues

1. Hot-reload doesn't work—instead, webpack-dev-server fallbacks to live-reload. — migration to react-refresh-webpack-plugin solved this issue.

@valerybugakov valerybugakov added ops & tools & dev frontend-platform Issues related to our frontend platform, owned collectively by our frontend crew. dx Issues and PRs related to developer experience concerns labels Jul 5, 2021
@valerybugakov valerybugakov self-assigned this Jul 5, 2021
Copy link
Contributor

@felixfbecker felixfbecker left a comment

Choose a reason for hiding this comment

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

Let's gooooo 💯

client/browser/config/webpack/base.config.ts Outdated Show resolved Hide resolved
client/browser/config/webpack/development.config.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
client/storybook/src/main.ts Outdated Show resolved Hide resolved
Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

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

Tried it locally, the performance gains are just awesome. Thanks for finally getting this ready!

Two minor things:

  • I think live reload is way slower, do we know why hot reload isn't working anymore? Do we know if we can get it fixed before the merge?
  • The log output from webpack contains quite a few stats, that are usually not super important when iterating on something and cause lots of log spam, can we teach webpack to be a bit quieter, especially in watch mode?

package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
client/storybook/src/main.ts Outdated Show resolved Hide resolved
@eseliger
Copy link
Member

eseliger commented Jul 5, 2021

Oh and the prod build in CI takes twice as long as before, is this expected?

Comment on lines +37 to +39
// It's not possible to use `WebpackDevServer.Configuration` here yet, because
// type definitions for the `webpack-dev-server` are not updated to match v4.
const developmentServerConfig = {
Copy link
Member Author

Choose a reason for hiding this comment

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

Created an issue to come back to this later: #22832

@valerybugakov valerybugakov marked this pull request as ready for review July 14, 2021 15:47
@valerybugakov valerybugakov moved this from In progress 🧑‍💻 to In review 👀 in [DEPRECATED] Frontend Platform :: Current Work Jul 14, 2021
@@ -130,24 +161,25 @@ const config = {
'suggest',
],
}),
new webpack.IgnorePlugin(/\.flow$/, /.*/),
Copy link
Member Author

Choose a reason for hiding this comment

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

This plugin doesn't make any difference to the output bundle but contributes ~5s to the build time. Tested with production build via bundle analyzer.

@valerybugakov valerybugakov added the dx-announce Tag PRs with this label to include it in the monthly DX email update label Jul 15, 2021
Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

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

Awesome work. Super happy with the massive performance improvements we get here! Also: big 👍 on the PR description, it answered all my questions!

Tried it out locally and works great, LGTM!
Could we get back timing information in sg start? I liked having some insight into the recompile timing and to see when recompilation starts. Currently it feels a bit black box-y.

client/web/webpack.config.js Show resolved Hide resolved
package.json Show resolved Hide resolved
client/web/webpack.config.js Show resolved Hide resolved
[DEPRECATED] Frontend Platform :: Current Work automation moved this from In review 👀 to Approved ✅ Jul 15, 2021
Copy link
Contributor

@vovakulikov vovakulikov left a comment

Choose a reason for hiding this comment

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

This works super nicely. With reach-hot-reload, we had a problem with @reach UI components. If you debug some popup element or any other element that works with reach UI React context then after hot reload update that just stops working. (I understood that because react context doesn't work really well with hot reload updates) And I had to update the page manually each time that happened.

With fast refresh that isn't a problem anymore. This works perfectly, thanks @valerybugakov

client/web/src/OpenSourceWebApp.tsx Show resolved Hide resolved
@valerybugakov valerybugakov merged commit e31288f into main Jul 19, 2021
[DEPRECATED] Frontend Platform :: Current Work automation moved this from Approved ✅ to Done 🎉 Jul 19, 2021
@valerybugakov valerybugakov deleted the vb/webpack-v5 branch July 19, 2021 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dx Issues and PRs related to developer experience concerns dx-announce Tag PRs with this label to include it in the monthly DX email update frontend-platform Issues related to our frontend platform, owned collectively by our frontend crew. web build perf
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants