Skip to content
This repository has been archived by the owner on Aug 10, 2022. It is now read-only.

Webpack guide updates for webpack 4 #5798

Merged
merged 13 commits into from
Mar 23, 2018

Conversation

iamakulov
Copy link
Contributor

@iamakulov iamakulov commented Feb 16, 2018

What's changed, or what was fixed?

  • Updated the guide for webpack 4
  • Fixed a few wording/formatting issues

Target Live Date: 2018-02-24 (webpack 4 release)

  • This has been reviewed and approved by (Addy)
  • I have run gulp test locally and all tests pass. (I’m on windows, and it’s hard here, sorry.)
  • I have added the appropriate type-something label.
  • I've staged the site and manually verified that my content displays correctly.

Please don’t merge this yet. I was unable to test some plugins with webpack 4 because they weren’t made compatible yet. Here’s what left to do:

  • Test the InlineChunkWebpackPlugin instructions and update them if necessary
  • Test the WebpackManifestPlugin instructions and update them if necessary

I’ll re-check the status of these plugins on the next week.

Feel free to review the remaining text meanwhile, it should be stable.

CC: @petele @addyosmani

@addyosmani
Copy link
Contributor

Thanks for working on the updates to this guidance for webpack 4, Ivan! My first question was going to be whether we wanted to align this with the stable launch but it looks like you've already thought about that :)

I'll take a read through and do a technical review shortly

Make sure to enable the `production` mode when you’re building your app for production.
This will make webpack apply optimizations like minification, removal of development-only code
in libraries, [and more](https://medium.com/webpack/webpack-4-mode-and-optimization-5423a6bc597a).

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about this paragraph including a bullet list of the optimizations that are applied through production mode, each linked to where you otherwise manually enable them from this guide?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IDK, have no opinion here. What’s the benefit/use of this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the benefit is making it clear to the reader what the mode does (in case just using production without anything else is enough), but we don't need to do that.

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 think the benefit is making it clear to the reader what the mode does

That’s a good point! I would probably link to the webpack docs instead once they are updated – it seems slightly weird for me to link to a section which has a “Skip this section if you’re using the production mode” notice. What do you think?


## Enable minification {: #enable-minification }

Note: if you’re using [webpack 4 with the production mode](#enable-the-production-mode), the bundle-level minification is already enabled. You’ll only need to enable loader-specific options.
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that we're still including the webpack 3 guidance but clearly calling out where 4 has guidance that supersedes it like this.

@@ -78,7 +97,7 @@ function render(data, target) {

<li>

The UglifyJS plugin minifies it into approximately the following:
A minificator compresses it into approximately the following:
Copy link
Contributor

Choose a reason for hiding this comment

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

A minifier? (I haven't come across the term minificator before)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, you’re right, thanks :–)

@petele
Copy link
Member

petele commented Feb 21, 2018

@addyosmani - please remove the DO NOT MERGE label when it's ready feel free to squash and merge yourself

@iamakulov
Copy link
Contributor Author

OK, so the webpack ecosystem isn’t quite ready yet:

Given this, I’d delay the article update for a week or two until these plugins are properly updated. I feel like existing webpack users would also delay their migration because of these plugins, so the article outdatedness won’t be such an issue.

I’d prepare a separate PR to add a note like

This article covers optimization strategies for webpack 3. A few parts of it aren’t necessary or work slightly differently with webpack 4. We’re waiting for the webpack ecosystem to stabilize, and then we’ll update this guide.

// webpack.config.js (for webpack 4)
module.exports = {
optimization: {
concatenateModules: true,
Copy link

Choose a reason for hiding this comment

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

This is the default in webpack 4 runnign in prod mode, so this shouldn't be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks! There’s actually a note about Webpack 4 in the beginning of the section, but it’s worded improperly, so I’ll update it.

In the end, the section will have a note like “If you use the production mode, just skip this section”.

@addyosmani
Copy link
Contributor

The HTMLWebpackPlugin won’t likely be updated for webpack 4 until the beginning of March: jantimon/html-webpack-plugin#823 (comment). We recommend this plugin in the article, and it’s the most popular solution for doing this kind of thing, so it’s not easy to replace.

There's a patched fork of the project in jantimon/html-webpack-plugin#823 (comment) that works with webpack 4, but I think we might want to wait for the actual upstream project to adopt this change so the fork doesn't end up being recommended for too long. Wdyt?

The WebpackManifestPlugin has a PR open for webpack 4, but there were no updates for the last 9 days: shellscape/webpack-manifest-plugin#118. We also recommend this plugin in the article, and it’s popular, so no easy replacement too.

Looks like we're waiting on some further movement to shellscape/webpack-manifest-plugin#128

Given this, I’d delay the article update for a week or two until these plugins are properly updated. I feel like existing webpack users would also delay their migration because of these plugins, so the article outdatedness won’t be such an issue.

This sounds very reasonable to me. We can check back in a few weeks to see if the status of the above two projects has improved before landing this.

I’d prepare a separate PR to add a note like

This article covers optimization strategies for webpack 3. A few parts of it aren’t necessary or work slightly differently with webpack 4. We’re waiting for the webpack ecosystem to stabilize, and then we’ll update this guide.

👍

@iamakulov
Copy link
Contributor Author

There's a patched fork of the project in jantimon/html-webpack-plugin#823 (comment) that works with webpack 4, but I think we might want to wait for the actual upstream project to adopt this change so the fork doesn't end up being recommended for too long. Wdyt?

Yeah! Exactly what I was thinking :–)

Looks like we're waiting on some further movement to shellscape/webpack-manifest-plugin#128

Good point, thanks! Didn’t notice there was an additional PR open

@mastilver
Copy link

Regarding html-webpack-plugin, I talked to @jantimon, he will release a major today. If not I will probably do it

About webpack-manifest-plugin we are finalising stuff and I will make that we have at least something out by today (even if it's only a broken rc...)

@iamakulov
Copy link
Contributor Author

iamakulov commented Feb 28, 2018

@mastilver Superb, glad to hear that! I’ll update the article tomorrow then.

I’ll have to find a replacement for InlineChunkWebpackPlugin – I haven’t seen any webpack 4 updates there – but that should be rather quickly.

@iamakulov
Copy link
Contributor Author

Resolved conflicts, proof-read the updated sections and replaced InlineChunkWebpackPlugin with InlineSourcePlugin. The latter plugin is also not upgraded yet, but there’s an PR which works: DustinJackson/html-webpack-inline-source-plugin#38

So, right now, I’m waiting for DustinJackson/html-webpack-inline-source-plugin#38 and shellscape/webpack-manifest-plugin#128. Once they are merged, we’re good to merge the article.

@addyosmani
Copy link
Contributor

SGTM. Looking at the latest updates on those issues, it appears we'll be waiting another short while before they get reviewed/landed.

@tailgo
Copy link

tailgo commented Mar 8, 2018

DustinJackson/html-webpack-inline-source-plugin has been updated. @iamakulov

@iamakulov
Copy link
Contributor Author

@tailgo Great, thanks for letting me know!

Yep, v0.0.10 works with webpack 4.

@addyosmani
Copy link
Contributor

Excellent. It looks like we're just waiting for the memory leak issues to be addressed in shellscape/webpack-manifest-plugin#128 now.

@iamakulov
Copy link
Contributor Author

iamakulov commented Mar 12, 2018

Yeah! Waiting for it.

If it doesn’t get fixed in a few days, I’ll vote to merge this anyway I believe – memory leak is a thing we can live with.

(Use an original repo instead of a fork)
`^` and `$` stopped working for some reason (tested on the training project)
@iamakulov
Copy link
Contributor Author

@addyosmani shellscape/webpack-manifest-plugin#128 isn’t merged yet, but I’d say let’s merge this. What do you think?

I’ve also updated the training project: GoogleChromeLabs/webpack-training-project#5

@addyosmani
Copy link
Contributor

@iamakulov For confirmation, the only issue folks will run into with the manifest plugin in the current state is they might run into race conditions. It should otherwise continue to function. Is that correct?

@mastilver
Copy link

@addyosmani Yeah, it's just a really rare race condition. I will ignore the test for webpack 4 then release
I will then try to make sure this can't happens with webpack 4

@addyosmani
Copy link
Contributor

Thank you for confirming, @mastilver! ⭐️

Copy link
Contributor

@addyosmani addyosmani left a comment

Choose a reason for hiding this comment

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

LGTM

@addyosmani addyosmani merged commit acbbe31 into google:master Mar 23, 2018
@iamakulov iamakulov deleted the webpack-guide-v4-updates branch March 23, 2018 20:50
@WebFundBot
Copy link

🎉 This has been pushed live to https://developers.google.com/web/

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants