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

[hold] upgrade: Webpack 5 #6496

Closed
wants to merge 1 commit into from
Closed

[hold] upgrade: Webpack 5 #6496

wants to merge 1 commit into from

Conversation

icirellik
Copy link
Contributor

@icirellik icirellik commented Oct 16, 2020

A quick upgrade to Webpack 5 brings with it a savings of ~100 KB gzipped. Along with all the other features of Webpack 5:

  • Better Tree-Shaking
  • Better Caching
  • Module Federation

Before:

webpack-4

After:

webpack-5

@icirellik icirellik self-assigned this Oct 16, 2020
@damassi
Copy link
Member

damassi commented Oct 16, 2020

@icirellik What does R and U mean?

@icirellik
Copy link
Contributor Author

Reminders of follow-up work.

@icirellik icirellik requested review from joeyAghion and dzucconi and removed request for eessex October 30, 2020 18:38
@icirellik icirellik changed the title [WIP] upgrade: Webpack 5 upgrade: Webpack 5 Oct 30, 2020
@icirellik icirellik marked this pull request as ready for review October 30, 2020 18:38
@icirellik icirellik requested a review from eessex October 30, 2020 18:39
// TODO: Why would these end up in the client bundle?
new webpack.IgnorePlugin({
resourceRegExp: /^react-relay-network-modern-ssr\/node8\/server/,
}),
Copy link
Member

Choose a reason for hiding this comment

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

I think this was added due to how Artsy/Router/buildServer|ClientApp used to be exported from reaction, but we import these things directly everywhere now so i think this is safe to remove.

fallback: {
"path": false,
"os": require.resolve("os-browserify/browser"),
"buffer": require.resolve("buffer/"),
Copy link
Member

Choose a reason for hiding this comment

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

What does this section + individual keys do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Webpack 5 no longer provides automatic browser polyfills. This section specifies the recommended polyfill to use for each node api we access in the browser.

@@ -56,29 +52,13 @@ export const clientDevelopmentConfig = {
],
},
plugins: [
new CaseSensitivePathsPlugin(),
Copy link
Member

Choose a reason for hiding this comment

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

This was recently added because it helped us track down a very pesky case sensitive bug. Why remove?

Copy link
Member

@damassi damassi Oct 30, 2020

Choose a reason for hiding this comment

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

If all of the following removals are due to plugin compat, we should comment them out with a

TODO: Reenable once webpack 5 is supported

unless there are comparable built-in webpack features we can leverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you explain the particular bug in more detail or provide a link to it? I am intimately familiar with Apples case-insensitive file system and the many issues that it creates in particular when renaming files in git. However git has options to address this already and wonder if we could instead leverage those.

Copy link
Member

@dzucconi dzucconi Oct 30, 2020

Choose a reason for hiding this comment

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

#6462 was the issue that prompted this — I had downcased a folder name accidentally against convention, but imports would work locally with alternate casing. I'd very much like to be warned about this, regardless of where it happens.

Copy link
Member

Choose a reason for hiding this comment

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

(Er, that's not the issue but rather the fix)

Copy link
Member

@damassi damassi left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on @icirellik 👍

A couple nonblocking q's / requests around commenting out incompatable webpack plugins so that we can remember to check up and reenable when ready for webpack 5 (versus deleting outright), but this pushes things forward nicely in the meantime.

This is a work in progress to try upgrading to webpack 5.
"@babel/preset-typescript": "7.10.4",
"@babel/register": "7.10.5",
"@babel/runtime": "7.11.2",
"@babel/cli": "^7.12.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be a good idea to extract these babel updates into their own PR if we can easily isolate.

@icirellik icirellik changed the title upgrade: Webpack 5 [hold] upgrade: Webpack 5 Dec 14, 2020
@icirellik
Copy link
Contributor Author

Waiting on: gregberge/loadable-components#636

@icirellik
Copy link
Contributor Author

Will create a new PR when @loadable is compatible with Webpack 5 async loading as this PR has become quite stale.

@icirellik icirellik closed this Jan 22, 2021
@zephraph zephraph deleted the upgrade-webpack-5 branch March 31, 2021 21:28
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.

4 participants