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

Upgrade to Webpack 2 #3460

Merged
merged 16 commits into from Feb 24, 2017
Merged

Upgrade to Webpack 2 #3460

merged 16 commits into from Feb 24, 2017

Conversation

@dennisoelkers
Copy link
Member

@dennisoelkers dennisoelkers commented Feb 2, 2017

Description

Motivation and Context

As webpack 1 is now officially deprecated, we are upgrading to webpack 2. The initial idea was to reduce asset size by employing tree shaking, but at the current state there is only a theoretical benefit as CommonJS imports are not statically analyzable and therefore unaffected by it. Right now this PR is only doing the required changes to be able to use webpack 2 (slight changes of the config syntax, removal of some now obsolete config parts, some minor changes of TS imports/exports due to stricter semantics) and updates of webpack/babel version numbers, no further optimization has been performed yet.

This is currently in progress, as it would require changes to plugin webpack configs.

How Has This Been Tested?

Web Interface and plugin assets have been bundled and quickly tested.

@edmundoa
Copy link
Member

@edmundoa edmundoa commented Feb 13, 2017

Looking through the opened webpack 2 issues I saw webpack/webpack#3265 that could affect us. I think we need to pay special attention to see if the public path still works for us after the upgrade.

@@ -14,7 +14,12 @@ const TARGET = process.env.npm_lifecycle_event;
process.env.BABEL_ENV = TARGET;

const BABELRC = path.resolve(ROOT_PATH, '.babelrc');
const BABELLOADER = 'babel-loader?cacheDirectory&extends=' + BABELRC;
const BABELOPTIONS = {

This comment has been minimized.

@edmundoa

edmundoa Feb 13, 2017
Member

Just to not forget about it: in order to use tree shaking we need to update the babelrc file to not transform ES2015 modules:

{
  presets: [
    ["es2015", { "modules": false }]
  ]
}

Source: https://babeljs.io/docs/plugins/preset-es2015/

This comment has been minimized.

@dennisoelkers

dennisoelkers Feb 21, 2017
Author Member

While I am not against that, I don't think we should do it here, as it is changing parsing semantics beyond the scope of this PR.

This comment has been minimized.

@edmundoa

edmundoa Feb 22, 2017
Member

Fair enough 👍

@edmundoa
Copy link
Member

@edmundoa edmundoa commented Feb 16, 2017

Can you please rebase against the current master? 🙂

@dennisoelkers
Copy link
Member Author

@dennisoelkers dennisoelkers commented Feb 16, 2017

Will do, thanks! :)

@dennisoelkers dennisoelkers force-pushed the webpack-2 branch from 1e06605 to 17e1e4c Feb 16, 2017
@edmundoa
Copy link
Member

@edmundoa edmundoa commented Feb 20, 2017

I tested executing the production build (using the graylog project and executing a mvn clean compile from IntelliJ) and the build failed:

[INFO] Running 'npm run build' in /foo/Workspace/graylog/project/master/graylog-plugin-map-widget
[INFO]
[INFO] > MapWidget@2.2.0-alpha.1 build /foo/Workspace/graylog/project/master/graylog-plugin-map-widget
[INFO] > webpack --bail
[INFO]
[INFO] Running in production mode
[ERROR] /foo/Workspace/graylog/project/master/graylog2-server/graylog2-web-interface/node_modules/webpack/lib/ExternalModuleFactoryPlugin.js:18
[ERROR] 			var dependency = data.dependencies[0];
[ERROR] 			                                  ^
[ERROR]
[ERROR] TypeError: Cannot read property '0' of undefined
[ERROR]     at ExternalModuleFactoryPlugin.<anonymous> (/foo/Workspace/graylog/project/master/graylog2-server/graylog2-web-interface/node_modules/webpack/lib/ExternalModuleFactoryPlugin.js:18:38)
[ERROR]     at handleExternal (/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/webpack/lib/ExternalModuleFactoryPlugin.js:25:32)
[ERROR]     at /foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/webpack/lib/ExternalModuleFactoryPlugin.js:81:24
[ERROR]     at handleExternals (/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/webpack/lib/ExternalModuleFactoryPlugin.js:78:5)
[ERROR]     at ExternalModuleFactoryPlugin.<anonymous> (/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/webpack/lib/ExternalModuleFactoryPlugin.js:79:5)
[ERROR]     at /foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/webpack/lib/NormalModuleFactory.js:159:3
[ERROR]     at NormalModuleFactory.applyPluginsAsyncWaterfall (/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/tapable/lib/Tapable.js:75:69)
[ERROR]     at NormalModuleFactory.create (/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/webpack/lib/NormalModuleFactory.js:144:8)
[ERROR]     at Compilation.process [as _addModuleChain] (/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/webpack/lib/Compilation.js:356:16)
[ERROR]     at Compilation.process [as addEntry] (/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/webpack/lib/Compilation.js:424:7)
[ERROR]     at SingleEntryPlugin.<anonymous> (/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/webpack/lib/SingleEntryPlugin.js:22:15)
[ERROR]     at Compiler.applyPluginsParallel (/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/tapable/lib/Tapable.js:107:14)
[ERROR]     at Compiler.compile (/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/webpack/lib/Compiler.js:394:7)
[ERROR]     at Compiler.<anonymous> (/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/webpack/lib/Compiler.js:173:9)
[ERROR]     at Compiler.readRecords (/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/webpack/lib/Compiler.js:302:10)
[ERROR]     at Compiler.<anonymous> (/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/webpack/lib/Compiler.js:170:8)
[ERROR]     at Compiler.next (/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/tapable/lib/Tapable.js:67:11)
[ERROR]     at Compiler.<anonymous> (/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/webpack/lib/node/NodeEnvironmentPlugin.js:23:3)
[ERROR]     at Compiler.next (/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/tapable/lib/Tapable.js:69:14)
[ERROR]     at Compiler.<anonymous> (/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/webpack/lib/CachePlugin.js:22:58)
[ERROR]     at Compiler.applyPluginsAsync (/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/tapable/lib/Tapable.js:71:13)
[ERROR]     at Compiler.run (/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/webpack/lib/Compiler.js:167:7)
[ERROR]     at processOptions (/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/webpack/bin/webpack.js:188:12)
[ERROR]     at Object.<anonymous> (/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node_modules/webpack/bin/webpack.js:192:1)
[ERROR]     at Module._compile (module.js:570:32)
[ERROR]     at Object.Module._extensions..js (module.js:579:10)
[ERROR]     at Module.load (module.js:487:32)
[ERROR]     at tryModuleLoad (module.js:446:12)
[ERROR]     at Function.Module._load (module.js:438:3)
[ERROR]     at Module.runMain (module.js:604:10)
[ERROR]
[ERROR] npm ERR! Darwin 16.4.0
[ERROR] npm ERR! argv "/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node/node" "/foo/Workspace/graylog/project/master/graylog-plugin-map-widget/node/node_modules/npm/bin/npm-cli.js" "run" "build"
[ERROR] npm ERR! node v6.9.2
[ERROR] npm ERR! npm  v4.0.3
[ERROR] npm ERR! code ELIFECYCLE
[ERROR] npm ERR! MapWidget@2.2.0-alpha.1 build: `webpack --bail`
[ERROR] npm ERR! Exit status 1
Copy link
Member

@edmundoa edmundoa left a comment

Apart from the inline comments, in my opinion we should still update the babel configuration to address this: #3460 (comment)

@@ -96,6 +96,7 @@ if (TARGET === 'start') {
},
plugins: [
new webpack.HotModuleReplacementPlugin(),
new webpack.NamedModulesPlugin(),

This comment has been minimized.

@edmundoa

edmundoa Feb 20, 2017
Member

Next time it would be very helpful adding a link or a little description of what a module does. I spent quite some time trying to figure out what NamedModulesPlugin does.

This comment has been minimized.

@dennisoelkers

dennisoelkers Feb 21, 2017
Author Member

👍

@@ -132,11 +130,6 @@ if (TARGET === 'build') {
process.env.NODE_ENV = 'production';
module.exports = merge(webpackConfig, {
plugins: [
new webpack.DefinePlugin({

This comment has been minimized.

@edmundoa

edmundoa Feb 20, 2017
Member

Why is the DefinePlugin obsolete? I didn't find anything in the webpack 2 migration guide or the documentation for that plugin.

Additionally, if it is obsolete, we should also remove it from the development configuration.

This comment has been minimized.

@dennisoelkers

dennisoelkers Feb 21, 2017
Author Member

It is explained here. The other usages of the DefinePlugin are used for something different, namely the red bubble in the web interface indicating that we are on dev, so we can keep them.

preLoaders: [
{ test: /\.js(x)?$/, loader: 'eslint-loader', exclude: /node_modules|public\/javascripts/ }
rules: [
{ test: /\.js(x)?$/, enforce: "pre", loader: 'eslint-loader', exclude: /node_modules|public\/javascripts/ }

This comment has been minimized.

@edmundoa

edmundoa Feb 20, 2017
Member

Minor nitpick: enforce should use single quotes.

This comment has been minimized.

@edmundoa

edmundoa Feb 20, 2017
Member

We also have some in the start task output, we should replace them as well for consistency.

This comment has been minimized.

@dennisoelkers

dennisoelkers Feb 21, 2017
Author Member

👍

This comment has been minimized.

@dennisoelkers

dennisoelkers Feb 22, 2017
Author Member

'process.env': {
NODE_ENV: JSON.stringify('production'),
},
}),
new webpack.optimize.UglifyJsPlugin({
minimize: true,

This comment has been minimized.

@edmundoa

edmundoa Feb 20, 2017
Member

We need to pass the minimize option into the loaders now, as apparently the UglifyJsPlugin does not do it any more: https://webpack.js.org/guides/migrating/#uglifyjsplugin-minimize-loaders

This comment has been minimized.

@dennisoelkers

dennisoelkers Feb 21, 2017
Author Member

Good point, thanks!

This comment has been minimized.

@dennisoelkers

dennisoelkers Feb 22, 2017
Author Member

@dennisoelkers
Copy link
Member Author

@dennisoelkers dennisoelkers commented Feb 22, 2017

Regarding the error you are getting: I addressed this in fd48e35. For some plugins (like the map widget plugin) which add their own webpack config statements, you need some minor adaptions. For the map widget plugin I have created the webpack-2 branch.

Copy link
Member

@edmundoa edmundoa left a comment

@dennisoelkers the changes look good, although I still have to verify that the asset paths are working as expected. Anyway, this PR cannot be merged until all plugins are adapted to webpack 2, as this will break the builds for everybody.

@edmundoa
Copy link
Member

@edmundoa edmundoa commented Feb 23, 2017

Path prefixes seem to work fine, I think the only missing task is to adapt all standard plugins.

@dennisoelkers
Copy link
Member Author

@dennisoelkers dennisoelkers commented Feb 24, 2017

PRs for the affected standard plugins have been created.

Copy link
Member

@edmundoa edmundoa left a comment

LGTM 👍

@edmundoa edmundoa merged commit a23b5f6 into master Feb 24, 2017
3 checks passed
3 checks passed
@garybot2
ci-web-linter Jenkins build graylog-pr-linter-check 1371 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@edmundoa edmundoa deleted the webpack-2 branch Feb 24, 2017
@edmundoa edmundoa mentioned this pull request Feb 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants