Upgrade to webpack 3 by spalger · Pull Request #14315 · elastic/kibana

spalger · 2017-10-04T21:59:57Z

Upgrades the optimizer to use webpack 3. Also introduces cache-loaders, which make builds much more efficient by storing loader caches on disk, in the optimize/.cache directory.

This also helps greatly with the memory issues people have been experiencing. I recommend that we just merge this into master, try it out for a little while, and see how it goes.

The only real departure from webpack 2 is the fact that it no longer checks the root of the Kibana repo when trying to resolve modules. It's not clear why it was ever doing this, but it seems that the only place that was using this feature was webpackShims, so they now use relative imports to load directly from the node_modules directory.

spalger · 2017-10-06T00:12:13Z

@jbudz do you know what impact the new optimize/.cache directory will have on upgrades and install? I assume it's another directory that might end up with root permissions and could be problematic. I'm happy to move it into optimize/bundles if you think that might help

jbudz · 2017-10-06T00:45:52Z

We chown the whole optimize dir here so I think we'll be good. If an optimize is triggered as a different user we'll have perm issues, but it'll be consistent with the existing permission issues

jbudz · 2017-10-06T00:51:33Z

We may want to create an empty directory so we don't run into #7458, but I don't think it'll have any benefit if we don't have the filenames beforehand. I think the right way to go is to put the optimize step behind a flag so it's only ran by the kibana server (and not the plugin installer, possibly only for packages), and thus the kibana user.

For reference, the issue I mentioned in the above comment is #8818.

sorry for all the words. tldr no regression, possible permission issues.

…ck-3

tylersmalley

Nothing major

tylersmalley · 2017-10-09T21:05:58Z

+            loader: 'jade-loader'
+          },
+          {
+            test: /\.json$/,


This can be removed

https://webpack.js.org/guides/migrating/#json-loader-is-not-required-anymore

tylersmalley · 2017-10-09T21:08:42Z

-          query: {
-            config: require.resolve('./postcss.config')
-          }
+    const devtool = this.sourceMaps;


A lot of these are only used once. Any reason for the assignment? To me, it doesn't appear to improve readability.

I really just wanted to know what this.* properties are being used for what settings, which I think this communicates, but I'm fine removing the renaming if you prefer.

tylersmalley · 2017-10-09T21:14:47Z

+        modules: [
+          'webpackShims',
+          'node_modules',
+          resolve(contextDir, 'webpackShims'),


I believe these two absolute paths are redundant.

Actually, they ensure that plugins will be able to use the webpackShims and node_modules from Kibana even when they are defined outside of the Kibana source directory.

Don't we create a symlink inside the plugins directory for those?

No, but even if we did webpack will use the real path of the module, not the link, to find the node_modules/webpackShims directories to match node.js functionality

tylersmalley · 2017-10-09T21:17:05Z

+              },
+            ],
          },
+          ...postLoaders.map(loader => ({


Looks like postLoaders were removed.

https://webpack.js.org/guides/migrating/#module-preloaders-and-module-postloaders-were-removed-

Yeah, which is why this adds enforce: 'post' to each loader definition.

…ck-3

tylersmalley · 2017-10-30T17:22:05Z

@spalger, this ready for another look?

spalger · 2017-10-30T17:22:40Z

@tylersmalley yes sir

tylersmalley · 2017-10-30T18:32:30Z

I have published snapshots of this branch:

https://download.elastic.co/kibana/staging/7.0.0-alpha1-SNAPSHOT-48a5214/kibana/kibana-7.0.0-alpha1-SNAPSHOT-amd64.deb
https://download.elastic.co/kibana/staging/7.0.0-alpha1-SNAPSHOT-48a5214/kibana/kibana-7.0.0-alpha1-SNAPSHOT-darwin-x86_64.tar.gz
https://download.elastic.co/kibana/staging/7.0.0-alpha1-SNAPSHOT-48a5214/kibana/kibana-7.0.0-alpha1-SNAPSHOT-linux-x86_64.tar.gz
https://download.elastic.co/kibana/staging/7.0.0-alpha1-SNAPSHOT-48a5214/kibana/kibana-7.0.0-alpha1-SNAPSHOT-windows-x86_64.zip
https://download.elastic.co/kibana/staging/7.0.0-alpha1-SNAPSHOT-48a5214/kibana/kibana-7.0.0-alpha1-SNAPSHOT-x86_64.rpm

Then, with one of the builds, installed an X-Pack snapshot

In addition to taking longer than expected (almost 6 minutes), it also produced an error:

Attempting to transfer from https://snapshots.elastic.co/downloads/kibana-plugins/x-pack/x-pack-7.0.0-alpha1-SNAPSHOT.zip
Transferring 271093694 bytes....................
Transfer complete
Retrieving metadata from plugin archive
Extracting plugin archive
Extraction complete
Optimizing and caching browser bundles...
Plugin installation was unsuccessful due to error "Optimizations failure.
   5250 modules

    WARNING in ./node_modules/cache-loader/dist/cjs.js?{"cacheDirectory":"/Users/tyler/Downloads/kibana-7.0.0-alpha1-SNAPSHOT-darwin-x86_64/optimize/.cache/699784d630fb8db5f4ef7d790b9ff02c0d919c38/less"}!./node_modules/css-loader?{"importLoaders":2}!./node_modules/postcss-loader/lib?{"config":{"path":"/Users/tyler/Downloads/kibana-7.0.0-alpha1-SNAPSHOT-darwin-x86_64/src/optimize/postcss.config.js"}}!./node_modules/less-loader/dist/cjs.js!./plugins/x-pack/plugins/searchprofiler/public/less/main.less
    (Emitted value instead of an instance of Error) autoprefixer: /Users/tyler/Downloads/kibana-7.0.0-alpha1-SNAPSHOT-darwin-x86_64/plugins/x-pack/plugins/searchprofiler/public/less/main.less:140:3: Gradient has outdated direction syntax. New syntax is like `to left` instead of `right`.

    WARNING in ./node_modules/cache-loader/dist/cjs.js?{"cacheDirectory":"/Users/tyler/Downloads/kibana-7.0.0-alpha1-SNAPSHOT-darwin-x86_64/optimize/.cache/699784d630fb8db5f4ef7d790b9ff02c0d919c38/less"}!./node_modules/css-loader?{"importLoaders":2}!./node_modules/postcss-loader/lib?{"config":{"path":"/Users/tyler/Downloads/kibana-7.0.0-alpha1-SNAPSHOT-darwin-x86_64/src/optimize/postcss.config.js"}}!./node_modules/less-loader/dist/cjs.js!./plugins/x-pack/plugins/searchprofiler/public/less/main.less
    (Emitted value instead of an instance of Error) autoprefixer: /Users/tyler/Downloads/kibana-7.0.0-alpha1-SNAPSHOT-darwin-x86_64/plugins/x-pack/plugins/searchprofiler/public/less/main.less:164:3: Gradient has outdated direction syntax. New syntax is like `to left` instead of `right`.

    ERROR in ./plugins/x-pack/plugins/ml/webpackShims/lodash.js
    Module not found: Error: Can't resolve 'node_modules/lodash/index.js' in '/Users/tyler/Downloads/kibana-7.0.0-alpha1-SNAPSHOT-darwin-x86_64/plugins/x-pack/plugins/ml/webpackShims'

…ck-3

tylersmalley · 2017-10-30T20:13:23Z

As Spencer pointed out on Slack, my issue was caused by not running X-Pack off of the corresponding branch.

Regarding the plugin installation/optimize time, it's consistent with master. However, that time seems to have ballooned at some point. We're now looking at ~8-9 minutes to install X-Pack on my machine.

tylersmalley

LGTM

kimjoar

LGTM. A couple nitpicks.

kimjoar · 2017-10-31T16:17:52Z

+          {
+            loader: 'css-loader',
+            options: {
+              importLoaders: 1 + preProcessors.length,


Maybe add a comment explaining why 1 + len?

kimjoar · 2017-10-31T16:21:55Z

-    if (env !== 'production') {
-      return [];
+      return webpackMerge(commonConfig, {
+        externals: {


nitpick, maybe pull in the comment above into:

if (this.env.context.env === 'development') { return webpackMerge(commonConfig, { // In the test env we need to add react-addons (and a few other bits) for the // enzyme tests to work. // https://github.com/airbnb/enzyme/blob/master/docs/guides/webpack.md externals: { 'react/lib/ExecutionEnvironment': true, 'react/addons': true, 'react/lib/ReactContext': true, } }); }

kimjoar · 2017-10-31T16:41:24Z

-        alias: transform(pkg.dependencies, function (aliases, version, name) {
-          if (name.endsWith('-loader')) {
-            aliases[name] = require.resolve(name);
+        alias: Object.keys(pkg.dependencies || {}).reduce((acc, key) => {


I removed resolveLoader entirely and the build succeeded. Not sure if this block is needed anymore? If it's still needed, maybe add a comment that clarifies its purpose?

You're right, I couldn't find the docs for this at first but webpack 2 changed the way loaders are resolved in the module rules: https://webpack.js.org/configuration/module/#useentry

In webpack 1 the loaders defined here were resolved relative to the file they were going to load, which meant that plugins in other projects could accidentally overwrite the loaders Kibana was trying to use, which is why the aliases were used to enforce proper resolution.

In webpack 2 loaders are now resolved relative to the webpackConfig.context, which is set to the root of the Kibana repo.

In webpack 1 the loaders defined here were resolved relative to the file they were going to load, which meant that plugins in other projects could accidentally overwrite the loaders Kibana was trying to use, which is why the aliases were used to enforce proper resolution. In webpack 2 loaders are now resolved relative to the webpackConfig.context, which is set to the root of the Kibana repo. See https://webpack.js.org/configuration/module/#useentry

epixa · 2017-11-06T21:14:21Z

@spalger I think we should backport this to 6.x. We should learn more about the problems this does or does not introduce quicker since a lot of manual QA is and will be done on 6.1, and backporting it early means we're not caught in a situation where we need to track down a long line of related PRs that may have been fixing bugs that this introduced, since we'll be backporting them as they are fixed. I think if there were truly glaring problems outstanding with this change, we would have caught them by now.

spalger · 2017-11-07T17:21:13Z

👍 @epixa

* [timelion] remove last remaining amd modules * [eslint-config-kibana] remove env.amd * [webpack] use absolute loader names * [webpack] remove absolute node_modules/ imports * [webpack] upgrade to webpack 3 * [uiFramework] make webpack build compatible with v3 * [eslint-import-resolver] use elastic/eslint-import-resolver-kibana#21 * [baseOptimizer] don't break when pkg has no dependencies * [optimize] remove unnecessary json-loader * [optimize] remove local references to webpack vars * [eslint] upgrade to eslint-import-resolver-kibana 0.9.0 * [baseOptimizer] comment tweaks * [baseOptimizer] remove loader pinning In webpack 1 the loaders defined here were resolved relative to the file they were going to load, which meant that plugins in other projects could accidentally overwrite the loaders Kibana was trying to use, which is why the aliases were used to enforce proper resolution. In webpack 2 loaders are now resolved relative to the webpackConfig.context, which is set to the root of the Kibana repo. See https://webpack.js.org/configuration/module/#useentry * [webpack] rely on kibana webpack shims before checking node_modules (cherry picked from commit f60639f)

* [timelion] remove last remaining amd modules * [eslint-config-kibana] remove env.amd * [webpack] use absolute loader names * [webpack] remove absolute node_modules/ imports * [webpack] upgrade to webpack 3 * [uiFramework] make webpack build compatible with v3 * [eslint-import-resolver] use elastic/eslint-import-resolver-kibana#21 * [baseOptimizer] don't break when pkg has no dependencies * [optimize] remove unnecessary json-loader * [optimize] remove local references to webpack vars * [eslint] upgrade to eslint-import-resolver-kibana 0.9.0 * [baseOptimizer] comment tweaks * [baseOptimizer] remove loader pinning In webpack 1 the loaders defined here were resolved relative to the file they were going to load, which meant that plugins in other projects could accidentally overwrite the loaders Kibana was trying to use, which is why the aliases were used to enforce proper resolution. In webpack 2 loaders are now resolved relative to the webpackConfig.context, which is set to the root of the Kibana repo. See https://webpack.js.org/configuration/module/#useentry * [webpack] rely on kibana webpack shims before checking node_modules

spalger added Team:Operations Kibana-Operations Team review v7.0.0 labels Oct 4, 2017

spalger requested review from jbudz and tylersmalley October 4, 2017 21:59

spalger force-pushed the upgrade/webpack-3 branch from b0a0d73 to 1761e87 Compare October 5, 2017 23:19

spalger added 7 commits October 5, 2017 16:41

[timelion] remove last remaining amd modules

116ec49

[eslint-config-kibana] remove env.amd

702953d

[webpack] use absolute loader names

d47e226

[webpack] remove absolute node_modules/ imports

455c1fb

[webpack] upgrade to webpack 3

73aa596

[uiFramework] make webpack build compatible with v3

742291f

[eslint-import-resolver] use elastic/eslint-import-resolver-kibana#21

eead1e4

spalger force-pushed the upgrade/webpack-3 branch from 1761e87 to eead1e4 Compare October 5, 2017 23:41

spalger added blocked and removed review labels Oct 5, 2017

[baseOptimizer] don't break when pkg has no dependencies

342aa1d

Merge branch 'master' of github.com:elastic/kibana into upgrade/webpa…

693f934

…ck-3

tylersmalley requested changes Oct 9, 2017

View reviewed changes

spalger added 5 commits October 10, 2017 16:01

Merge branch 'master' of github.com:elastic/kibana into upgrade/webpa…

904b05f

…ck-3

[optimize] remove unnecessary json-loader

aeedff1

[optimize] remove local references to webpack vars

52b7f53

Merge branch 'master' of github.com:elastic/kibana into upgrade/webpa…

19f4723

…ck-3

Merge branch 'master' of github.com:elastic/kibana into upgrade/webpa…

48a5214

…ck-3

spalger mentioned this pull request Oct 30, 2017

[webpack] upgrade to webpack 3 elastic/eslint-import-resolver-kibana#21

Merged

Merge branch 'master' of github.com:elastic/kibana into upgrade/webpa…

a590af4

…ck-3

spalger force-pushed the upgrade/webpack-3 branch from 2913568 to a590af4 Compare October 30, 2017 19:44

tylersmalley approved these changes Oct 30, 2017

View reviewed changes

kimjoar approved these changes Oct 31, 2017

View reviewed changes

spalger added review and removed blocked labels Oct 31, 2017

spalger added 3 commits October 31, 2017 12:45

[eslint] upgrade to eslint-import-resolver-kibana 0.9.0

fe4c327

[baseOptimizer] comment tweaks

8336b08

jbudz approved these changes Oct 31, 2017

View reviewed changes

[webpack] rely on kibana webpack shims before checking node_modules

c3f0f30

spalger merged commit f60639f into elastic:master Oct 31, 2017

spalger deleted the upgrade/webpack-3 branch October 31, 2017 23:31

tylersmalley mentioned this pull request Nov 6, 2017

Lazy optimizer triggering out of memory exception #14221

Closed

spalger added the v6.1.0 label Nov 7, 2017

tylersmalley added the release_note:enhancement label Nov 16, 2017

Conversation

spalger commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spalger commented Oct 6, 2017

Uh oh!

jbudz commented Oct 6, 2017

Uh oh!

jbudz commented Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tylersmalley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tylersmalley commented Oct 30, 2017

Uh oh!

spalger commented Oct 30, 2017

Uh oh!

tylersmalley commented Oct 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tylersmalley commented Oct 30, 2017

Uh oh!

tylersmalley left a comment

Choose a reason for hiding this comment

Uh oh!

kimjoar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epixa commented Nov 6, 2017

Uh oh!

spalger commented Nov 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

spalger commented Oct 4, 2017 •

edited

Loading

jbudz commented Oct 6, 2017 •

edited

Loading

tylersmalley commented Oct 30, 2017 •

edited

Loading