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

Webpack 4: Try migrating to WebPack 4 #5267

Merged
merged 2 commits into from
Mar 30, 2018
Merged

Webpack 4: Try migrating to WebPack 4 #5267

merged 2 commits into from
Mar 30, 2018

Conversation

youknowriad
Copy link
Contributor

This is not ready yet because extract-text-webpack-plugin is not available yet as a stable release for webpack 4.

I'm seeing some warnings in the console of Gutenberg related to source maps. Also not seeing the big performance improvements everyone is talking about.

@youknowriad youknowriad self-assigned this Feb 26, 2018
@@ -143,7 +144,7 @@ const config = {
}, {} )
),
output: {
filename: '[basename]/build/index.js',
filename: '[name]/build/index.js',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to update this, basename was not working. I wonder if it's because of CustomTemplatedPathPlugin?

Copy link
Member

Choose a reason for hiding this comment

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

Possible that the plugin needs to be updated to support the new version of Webpack.

@mtias mtias added the Framework Issues related to broader framework topics, especially as it relates to javascript label Mar 7, 2018
@aduth
Copy link
Member

aduth commented Mar 20, 2018

Related: #5687, WordPress/packages#93

package.json Outdated
@@ -77,7 +77,7 @@
"eslint-plugin-jsx-a11y": "6.0.2",
"eslint-plugin-react": "7.5.1",
"expose-loader": "0.7.3",
"extract-text-webpack-plugin": "3.0.0",
"extract-text-webpack-plugin": "4.0.0-alpha.0",
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/webpack-contrib/extract-text-webpack-plugin/releases/tag/v4.0.0-beta.0 - beta is out, but still not a stable one. We probably should proceed anyway.

@@ -127,6 +126,8 @@ class CustomTemplatedPathPlugin {
}

const config = {
mode: process.env.NODE_ENV === 'production' ? 'production' : 'development',
Copy link
Member

Choose a reason for hiding this comment

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

Interesting

@gziolo
Copy link
Member

gziolo commented Mar 21, 2018

I would say, let's finish work on @wordpress/custom-templated-path-webpack-plugin, update all deps again and merge it 👍

@aduth
Copy link
Member

aduth commented Mar 21, 2018

Relevant: webpack-contrib/extract-text-webpack-plugin#749

Appears there may be an official deprecation of extract-text-webpack-plugin in favor of mini-css-extract-plugin.

@gziolo
Copy link
Member

gziolo commented Mar 21, 2018

Yes, they officially recommend this new plugin for Webpack >= 4.0.0. We should update this PR.

@aduth
Copy link
Member

aduth commented Mar 21, 2018

Merged #5687 into this branch. I'll rebase and look at bringing in mini-css-extract-plugin.

@aduth
Copy link
Member

aduth commented Mar 22, 2018

Rebased to resolve conflicts. I tried pulling in mini-css-extract-plugin, but it began to turn into a rabbit hole, since it's not very compatible with our multiple-output configuration of extracting block CSS independently (webpack-contrib/mini-css-extract-plugin#45). I think there might be an opportunity to consolidate these, but it'd probably be part of a broader effort to create separate bundles per block (#4841, #2756, #2751, #5445). As such, since the beta version of extract-text-webpack-plugin seems to work okay, I'd be inclined to use it for the initial upgrade.

I'd also tinkered with some additional settings for Lodash, since only the lodash-es variation opts in to the new Webpack 4 sideEffects flag for improved tree shaking. This didn't seem to have an improvement in my brief testing, though I didn't investigate enough to determine where it was going wrong. I suspected a conflict with babel-plugin-lodash, but even disabling this there was no difference. I've been contemplating pulling out Lodash as a defined external anyways, so again I think this merits some separate explorations.

@aduth
Copy link
Member

aduth commented Mar 30, 2018

I was waiting on the previous release to move forward on this one. Rebased to resolve conflicts, verified development and production builds, and incremental rebuild (which is very fast by the way!).

Suggestions for future pull requests:

  • Replace extract-text-webpack-plugin with mini-css-extract-plugin
  • Either alias Lodash to lodash-es for sideEffects: true benefit, or define Lodash as an external (been on my mind lately, needs compat consideration with Underscore.js)

@aduth aduth merged commit 7f406fb into master Mar 30, 2018
@aduth aduth deleted the try/webpack4 branch March 30, 2018 20:27
break;

default:
config.devtool = 'source-map';
Copy link
Member

Choose a reason for hiding this comment

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

Did we intend to remove the devtool here? Currently investigating issues with sourcemaps, doesn't seem like this should have been impacted by the upgrade (aside from accommodating within new pattern for assigning environment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I originally removed all "dev" related setup because while reading about WebPack, I thought the idea was that the "mode" was taking care of all these things automatically. Not the source map I guess

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants