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

Update to webpack 4 #809

Merged
merged 20 commits into from Apr 25, 2018
Merged

Update to webpack 4 #809

merged 20 commits into from Apr 25, 2018

Conversation

@edmorley
Copy link
Member

edmorley commented Apr 20, 2018

Notable changes:

  • Major version updates of webpack, webpack-dev-server and optimize-css-assets-webpack-plugin.
  • Uses the new webpack 4 mode option.
  • Removed @neutrinojs/chunk in favour of the splitChunks feature.
  • Removed @neutrinojs/babel-minify in favour of the faster uglify-es (the fake vendor entrypoint should now be removed from people's .neutrinorc.js to avoid a bigger build then necessary).
  • Removed redundant ModuleConcatenationPlugin usage.
  • Removed default of NODE_ENV from @neutrinojs/env.
  • Replaced extract-test-webpack-plugin with mini-css-extract-plugin.
  • Stopped pinning webpack-sources to v1.0.1.
  • Added checks to warn about changed web preset minify options.
  • Added checks to enforce that users remove any legacy vendor entrypoints.

For more details see the individual commit messages (most of the intermediate commits will fail to run standalone, but it still seemed preferable to split them up for easier review).

See here for the webpack 4 changelog:
https://github.com/webpack/webpack/releases/tag/v4.0.0

I'd love to have any and all feedback - including:

  • what effect this branch has on build times/sizes compared to Neutrino 8
  • how uglify-es vs babel-minify compares for your use-cases

To try this branch out locally with create-project:

  1. git clone https://github.com/mozilla-neutrino/neutrino-dev.git
  2. cd neutrino-dev && git checkout webpack-4
  3. If this isn't a fresh clone, run: rm -rf node_modules/ packages/*/node_modules (works around what appears to be a yarn workspaces bug, where stray webpack 3.x directories were left behind, causing errors about missing json-loader)
  4. yarn && yarn link:all
  5. Ensure the yarn global bin directory is on PATH
  6. cd .. && create-project <new directory name> and answer the prompts
  7. cd <new directory name>
  8. Try out yarn {start, build, lint, test} as you would normally

Alternatively to use with an existing Neutrino 8 project:

  1. Follow steps 1-5 above.
  2. cd <path to existing Neutrino 8 project>
  3. Remove neutrino and @neutrinojs/* packages from package.json, and if there are any direct dependencies on webpack, webpack-dev-server, html-webpack-plugin or similar (eg since you import them in .neutrinorc.js for advanced customisation) then adjust their versions to the latest available.
  4. rm -rf yarn.lock node_modules (to prevent leftover hoisted webpack 3 packages from causing hard to debug errors)
  5. yarn && yarn link neutrino @neutrinojs/... @neutrinojs/... (replace @neutrinojs/... with whichever presets were previously referenced in package.json - no need to list every Neutrino preset, just the top level dependencies)
  6. Try out yarn {start, build, lint, test} as you would normally
  7. Once finished testing, to switch back to the published packages:
    • in the project that uses Neutrino: revert the changes to package.json and yarn.lock, then rm -rf node_modules && yarn
    • in the neutrino-dev repository: yarn lerna exec yarn unlink

Useful resources for reviewing:

Left to do:

  • Decide whether to use uglify-es or babel-minify by default (I'm leaning towards the former, see #748).
  • Decide whether to allow choosing minifier in the web preset, or if @neutrinojs/babel-minify (presuming we pick uglify-es) should be completely separated to reduce dependencies/complexity. (Especially since a future minimizer-webpack-plugin might mean any custom handling in the web preset has to be rewritten anyway.)
  • Make the relevant minification changes based on those decisions.
  • Keep an eye on jantimon/html-webpack-plugin#880 and webpack-contrib/karma-webpack#322, and remove the workarounds for them if fixed prior to this being merged.

Fixes #737.
Fixes #802.
Closes #748.
Closes #768.
Closes #769.
Closes #766.

@edmorley edmorley self-assigned this Apr 20, 2018
@edmorley edmorley requested review from eliperelman and timkelty Apr 20, 2018
@edmorley

This comment has been minimized.

Copy link
Member Author

edmorley commented Apr 20, 2018

I've updated the above instructions for trying out this branch to prevent leftover packages from causing problems. (If you see errors like TypeError: dep.getResourceIdentifier is not a function or Error: Cannot find module 'json-loader' it means there's a mixture of webpack 3 and 4 packages being used - the revised steps should avoid this.)

@eliperelman

This comment has been minimized.

Copy link
Member

eliperelman commented Apr 21, 2018

Preliminary builds using current and updated neutrino-preset-mozilla-frontend-infra on taskcluster-web:

screen shot 2018-04-21 at 10 16 00 am

screen shot 2018-04-21 at 10 24 10 am

Build time went from 154 seconds to 46 seconds. Incredible.


I noticed that in my updated preset, since the chunking logic is gone, I can no longer do this:

screen shot 2018-04-21 at 10 28 14 am

Does this mean that .chunkFilename(`[name].[chunkhash].${cacheVersion}.js`) should work properly for those chunks now?

@edmorley

This comment has been minimized.

Copy link
Member Author

edmorley commented Apr 21, 2018

Thank you for trying it out - nice improvement in build time! :-)

I think there's more caching going on by default now (node_modules/.cache) - how do the second build times compare? Also, how long does webpack 4 + {minify: { babel: true }} take compared to the default of uglify-es?

Does this mean that .chunkFilename([name].[chunkhash].${cacheVersion}.js) should work properly for those chunks now?

Yeah I'm pretty sure that should work.

Re the automatically generated chunk filenames - since this PR sets optimization.splitChunks.name to false in production, we end up with filenames like 1.ceddedc0defa56bec89f.js rather than the default of vendors~index~page2.b694ee990c08e6be6a33.js. Now the former avoids cache busting just because another entrypoint was added, however isn't very friendly.

As such, I was thinking of filing an issue against webpack asking for the default when name is false to instead include the name of the cacheGroup - which in this case is vendor. This would give filenames more like vendors~1.b694ee990c08e6be6a33.js. I haven't yet played around with passing functions to name, to see if we could do this ourselves in the meantime.

@eliperelman

This comment has been minimized.

Copy link
Member

eliperelman commented Apr 21, 2018

taskcluster-tools, using the same current and updated preset, removing the vendor bundle this time, prior to the updated build:

screen shot 2018-04-21 at 10 49 44 am

The second screenshot is rather large, so splitting in 4:

screen shot 2018-04-21 at 11 04 44 am

screen shot 2018-04-21 at 11 05 20 am

screen shot 2018-04-21 at 11 05 36 am

screen shot 2018-04-21 at 11 05 51 am


From 431 seconds to 61 seconds. From 7 minutes to 1 minute. Amazing.

@eliperelman

This comment has been minimized.

Copy link
Member

eliperelman commented Apr 21, 2018

Build summaries:

project webpack primed minifer time v3 improvement
taskcluster-web v3 no babel 154s -
taskcluster-web v4 no uglify 46s 70.1%
taskcluster-web v4 yes uglify 25s 83.8%
taskcluster-web v4 no babel 88s 42.9%
taskcluster-web v4 yes babel 84s 45.5%
taskcluster-tools v3 no babel 431s -
taskcluster-tools v4 no uglify 61s 85.9%
taskcluster-tools v4 yes uglify 41s 90.5%
taskcluster-tools v4 no babel 168s 61.0%
taskcluster-tools v4 yes babel 162s 62.4%

Takeaways:

  • Uglify is significantly faster than Babel minify.
  • Uglify is also significantly better with incremental builds over non-primed build. Babel minify shows no real statistical improvement in incremental builds.
Copy link
Member

eliperelman left a comment

Whew, finally made it through everything!

Everything here looks good to me, with the caveat that this will need to be rebased due to the uglify/babel minification discussions, and the image minification middleware patch.

Looks great so far. We will see where webpack-chain v5 and other things take us.

edmorley added 15 commits Apr 20, 2018
Since webpack 4 now sets a default entry-point of `./src`, causing
the error message to change for this test. (It doesn't seem worth
creating the file to confirm it works, since from the error message
we can tell the test got far enough that Neutrino correctly didn't
complain about an explicit entry-point not being set.)
* Update webpack to ^4.6.0
* Update webpack-chain to ^4.6.0
* Update webpack-dev-server to ^3.1.3
* Update optimize-css-assets-webpack-plugin to ^4.0.0
Bases `mode` on `NODE_ENV`, falling back to `'development'` for
anything but production, since otherwise minification (and more)
would be enabled during testing. This is the same approach used in
create-react-app's WIP webpack 4 PR.

For the effects `mode` has, see:
https://webpack.js.org/concepts/mode/
https://github.com/webpack/webpack/blob/v4.6.0/lib/WebpackOptionsDefaulter.js#L26-L294
https://github.com/webpack/webpack/blob/v4.6.0/lib/WebpackOptionsApply.js#L301-L342
Adds the `mode` and `optimizations` documentation from:
neutrinojs/webpack-chain#51
webpack 4 now has an intelligent chunking feature that replaces
`CommonsChunkPlugin` and avoids the need to create the fake `vendor`
entrypoint to separate out third-party dependencies:
https://webpack.js.org/plugins/split-chunks-plugin/

In addition, several of the plugins previously used are automatically
enabled when `mode` is development:
https://github.com/webpack/webpack/blob/v4.6.0/lib/WebpackOptionsDefaulter.js#L250-L259
https://github.com/webpack/webpack/blob/v4.6.0/lib/WebpackOptionsApply.js#L325-L328
(Note this differs slightly from our previous implementation, but if
this results in long-term caching issues, issues should be filed to
change the defaults upstream, and only set here as a last resort).

However since `splitChunks` creates an unknown number of chunks with
unpredictable filenames, it's no longer possible to pass an exact
chunks list to `html-webpack-plugin`'s `chunks` option. We also can't
omit the `chunks` option entirely, since for multi-entrypoint builds
that would result in all assets being included for every page.

As such, until `html-webpack-plugin` natively supports `splitChunks`,
we have to use `html-webpack-include-sibling-chunks` to dynamically
generate the `chunks` list:
https://github.com/fenivana/html-webpack-include-sibling-chunks-plugin

With the switch to `splitChunks`, `@neutrinojs/chunk` becomes simple
enough that it's not really worth keeping separate from the web preset,
particularly given that it relies on the sibling chunks workaround to
actually be useable anyway.

Note that webpack enables `splitChunks` in development too (which is
great for dev-prod parity, and fixes a bug I was hitting migrating
an app to Neutrino 8), which is why our overrides are no longer
inside the `NODE_ENV === 'production'` block.
Previously users were encouraged to create a fake `vendor` entrypoint
which worked in conjunction with `@neutrinojs/chunk` to separate out
third-party dependencies to a vendor chunk.

However with `splitChunks`, manually specifying this entrypoint will
actually result in larger builds - so we must ensure users correctly
update their configuration.
`uglify-js` (used by webpack 3) doesn't support ES6, so previously the
web preset used `babel-minify` instead, to avoid having to transpile
the generated javascript down to ES5 before minification.

However webpack 4 now uses the new `uglify-es`, which does support ES6,
and is much faster than `babel-minify`. As such, the web preset now
defaults to `uglify-es` (which webpack 4 enables by default when `mode`
is production).

In addition, `babel-minify-webpack-plugin` has been found to be buggy
when combined with `webpack-sources` > `1.0.1`, which required hacky
workarounds in the form of yarn `resolutions` overrides and a forked
version of `babel-minify-webpack-plugin`. This means we're on an old
version of `webpack-sources`, which is missing fixes and performance
improvements.

It's likely before this PR is merged (or else before Neutrino 9 is
released) that `@neutrinojs/babel-minify` will be completely detached
from the web preset (and the workarounds removed) - but for now I've
left it in, for easier A/B testing of the two minifiers.

For more context, see:
#748

It's also looking like we may want to adjust `webpack-chain` in the
future, so that `optimization.minimizer` behaves like `plugins` and
can have named entries for easier overriding. (To allow setting both
JS and CSS minifiers there at once - especially given webpack 5 may
also include a CSS minifier there too.)
Since the former is deprecated for CSS extraction, and is not
compatible with webpack 4. The new plugin has a much simpler API:
https://github.com/webpack-contrib/mini-css-extract-plugin

I've also chosen to remove `css-hot-loader` - since it's unnecessary
when using `style-loader` in development. ie option (2) from #802.
(Once `mini-css-extract-plugin` supports HMR we can stop using
`style-loader` entirely.)

NB: `mini-css-extract-plugin` doesn't support having multiple plugin
instances, so now we only have one instance for both standard CSS and
module CSS. This will need testing to ensure everything still works.
Disabling `optimization.splitChunks` prevents hangs when running
`yarn test`, and disabling `optimization.runtimeChunk` (which is
enabled by the web preset) fixes zero tests being run.

Example hang:
https://travis-ci.org/mozilla-neutrino/neutrino-dev/jobs/369061914#L833
There's still a lot left untested, but this improves coverage
significantly in the meantime.
We're going to stick with webpack 4's default of uglify-es for now,
since it's much faster than babel-minify. Between that and the new
webpack `optimization.minimize` and `optimization.minimizer` options,
it's simpler to just document how to override the defaults directly,
rather than provide a separate preset per alternative minifier.
It was required to work around a source map bug when using newer
`webpack-sources` with `babel-minify-webpack-plugin`:
webpack-contrib/babel-minify-webpack-plugin#68

However now that we're using `uglify-es` instead, we can stop pinning
the `webpack-sources` version and pick up some of the bug/perf fixes:
https://github.com/webpack/webpack-sources/releases
@edmorley edmorley force-pushed the webpack-4 branch from 1518615 to ca030ac Apr 24, 2018
@edmorley

This comment has been minimized.

Copy link
Member Author

edmorley commented Apr 24, 2018

PR rebased and conflicts from master resolved, then 65f77a3 and 8bc0d3b added to remove @neutrinojs/babel-minify and the webpack-sources pinning.

@edmorley edmorley requested a review from eliperelman Apr 24, 2018
@eliperelman

This comment has been minimized.

Copy link
Member

eliperelman commented Apr 25, 2018

Updated stats

project webpack primed minifer time v3 improvement
taskcluster-web v3 no babel 154s -
taskcluster-web v4 no uglify 46s 70.8%
taskcluster-web v4 yes uglify 23s 85.1%
taskcluster-web v4 no babel FAILED -
taskcluster-web v4 yes babel FAILED -
taskcluster-tools v3 no babel 431s -
taskcluster-tools v4 no uglify 76s 82.4%
taskcluster-tools v4 yes uglify 36s 91.7%
taskcluster-tools v4 no babel FAILED -
taskcluster-tools v4 yes babel FAILED -

Looks like numbers are comparable within margin of error.


Looks like the Babel minify error is back with the sources unpinned:

Error: original.line and original.column are not numbers -- you probably meant to
omit the original mapping entirely and only map the generated position. If so,
pass null for the original mapping instead of an object with empty or null values.

Copy link
Member

eliperelman left a comment

Code lgtm, but it looks like we may have webpack-sources back, at least for now.

const styleMinify = require('@neutrinojs/style-minify');
const loaderMerge = require('@neutrinojs/loader-merge');
const devServer = require('@neutrinojs/dev-server');
const { join } = require('path');
const { resolve } = require('url');
const merge = require('deepmerge');
const HtmlWebpackIncludeSiblingChunksPlugin = require('html-webpack-include-sibling-chunks-plugin')

This comment has been minimized.

Copy link
@eliperelman

eliperelman Apr 25, 2018

Member

Missing trailing semicolon. Thanks eslint!

@edmorley

This comment has been minimized.

Copy link
Member Author

edmorley commented Apr 25, 2018

but it looks like we may have webpack-sources back, at least for now.

Being able to remove that workaround for me was another of the benefits of dropping official babel-minify support - particularly since further webpack-sources performance improvements are coming (eg webpack/webpack-sources#23). Instead of holding everyone back on an old version just in case someone wants to use babel-minify, I think we should instead document the bug and any potential workarounds in the new web preset docs instructions for how to use babel-minify.

Also my hunch is that the only reason we're seeing the sourcemap errors with babel-minify is because of the exact devtool and babel-minify combination we're using. If we can find a devtool combination that doesn't give this error, then perhaps we can tell people to use that, avoiding the need for yarn resolutions fields or similar.

I'm going to create a minimal testcase repository without Neutrino to give the babel-minify-webpack-plugin maintainers something more to go on (in retrospect doing this sooner would have increased the chance of webpack-contrib/babel-minify-webpack-plugin#68 being fixed).

@edmorley

This comment has been minimized.

Copy link
Member Author

edmorley commented Apr 25, 2018

Or alternatively removing the babel-minify instructions entirely and replacing them with a mention that it's just too buggy and not recommended.

@edmorley

This comment has been minimized.

Copy link
Member Author

edmorley commented Apr 25, 2018

So re-reading the babel-minify-webpack-plugin issue I see that it occurs for all source map variants apart from cheap (and cheap is pointless when minifying, since everything is on one line), so I guess that leaves telling people to just disable sourcemaps in production if using babel-minify. (Which seems reasonable; it's not our fault it's buggy)

@eliperelman

This comment has been minimized.

Copy link
Member

eliperelman commented Apr 25, 2018

OK, I can agree with that. Let's just not support it for now with a warning.

edmorley added 5 commits Apr 25, 2018
So they catch the `false` case now too.
Now that the default `NODE_ENV` entry has been removed, the plugin
has nothing to do unless the user had passed `env` in the preset
options.
@edmorley edmorley requested a review from eliperelman Apr 25, 2018
@edmorley edmorley merged commit 4284ce4 into master Apr 25, 2018
3 checks passed
3 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@edmorley edmorley deleted the webpack-4 branch Apr 25, 2018
edmorley added a commit to edmorley/neutrino that referenced this pull request Apr 25, 2018
Since webpack 4 now loads the plugin by default in development.

Leftover from neutrinojs#809.
edmorley added a commit that referenced this pull request Apr 25, 2018
Since webpack 4 now loads the plugin by default in development.

Leftover from #809.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.