Build fails with webpack 4

[flipchart]

Upgraded my (working) project from webpack 3 to webpack 4, and the build now fails with the below error:

ERROR in ./node_modules/@ckeditor/ckeditor5-basic-styles/src/bold.js
Module build failed: TypeError: Cannot read property 'translateSource' of undefined
    at Object.translateSourceLoader (.\node_modules\@ckeditor\ckeditor5-dev-webpack-plugin\lib\translatesourceloader.js:16:22)

Looks like it's due to using the deprecated this.options value. See webpack 4 migration guide

[ma2ciek]

This looks bad as we need some context being shared through loader and plugin. I'll look at this.rootContext to see whether it cover our needs.

[Reinmar]

BTW, if possible, it'd be good if the new version of our plugin worked with Webpack3 and Webpack4.

[flipchart]

@Reinmar while admirable, I'm not sure it's entirely necessary - the new version ts-loader (Typescript loader) is incompatible with webpack 4. I'm not sure how much work you're still doing to this loader (i.e. new features), so maintaining webpack 3 compatibility might not be worth it. I'd release a new major version with webpack 4 compatibility and have people stay on the current version for webpack 3. Just my 2c

[ma2ciek]

I've found a workaround and maybe an even better solution to pass options directly to the loader, but there are few things to be done to migrate to webpack@4.

1. https://webpack.js.org/api/resolver/#resolve-context-string-request-string- webpack's resolver doesn't work at v4. we'd need to add an issue upstream or find another way to solve it - we use it to get the relative path for ckeditor5-core package that contains core translations.
2. Webpack 4 comes with webpack-cli package that we'd need to add to package.json as we run it within CLI.
3. Webpack 4 introduces Tap plugins. (https://medium.com/webpack/webpack-4-migration-guide-for-plugins-loaders-20a79b927202). The migration isn't hard but it's backward incompatible so we can do it later (next version of webpack should land in ~June)
4. Webpack 4 introduces modes. Production mode comes with UglifyJs2 and ModuleConcatenationPlugin. We can consider switching to modes as they will be commonly used by the community. As @oleq has found, new webpack with UglifyJs2 improves compilation time a bit.

[Reinmar]

https://webpack.js.org/api/resolver/#resolve-context-string-request-string- webpack's resolver doesn't work at v4. we'd need to add an issue upstream or find another way to solve it - we use it to get the relative path for ckeditor5-core package that contains core translations.

Let's prioritise this because it sounds like a blocking issue. Could you start from this? Let's at least know why it doesn't work and reporting it may help to learn that.

Webpack 4 comes with webpack-cli package that we'd need to add to package.json as we run it within CLI.

So, since W4, installing the webpack package doesn't install the webpack binary? Is this why this new pkg is needed?

Webpack 4 introduces Tap plugins. (https://medium.com/webpack/webpack-4-migration-guide-for-plugins-loaders-20a79b927202). The migration isn't hard but it's backward incompatible so we can do it later (next version of webpack should land in ~June)

Based on what @flipchart and you wrote here, W4 is a pretty big release. We're right now at a point when we can do bigger changes in how our plugins are proposed but this will be harder soon, so I'd rather focus on being up to date and safe. Also, as @flipchart mentioned, backwards compatibility shouldn't be our main goal.

Webpack 4 introduces modes. Production mode comes with UglifyJs2 and ModuleConcatenationPlugin. We can consider switching to modes as they will be commonly used by the community. As @oleq has found, new webpack with UglifyJs2 improves compilation time a bit.

I think this is a really good idea. We should make the experience as smooth as possible which often means sticking to common options. We can, therefore, consider replacing babel-preset-minify with UglifyJs2, and update the configuration in general.

[ma2ciek]

Let's prioritise this because it sounds like a blocking issue. Could you start from this? Let's at least know why it doesn't work and reporting it may help to learn that.

I'll try to reproduce it on a simple and empty repo and report it.

So, since W4, installing the webpack package doesn't install the webpack binary? Is this why this new pkg is needed?

Yes, W4 provides binaries by the webpack-cli package.

Regarding the rest, I'll try to make these changes after solving the first issue.

[ma2ciek]

I've managed to port the whole CKEditorWebpackPlugin to Webapack 4.

But I hit a problem with the new minifier. It breaks the editor with the image plugin.

webpack --mode production

Bundle size: 409 KiB

Error at sample/index.html (at least in Chrome, Firefox)

TypeError: Assignment to constant variable.
    at n (utils.js:144)
    at vs.Hm (imagecaptionediting.js:187)
    at vs.fire (emittermixin.js:196)
    at vs._testAndFire (downcastdispatcher.js:453)
    at vs.convertInsert (downcastdispatcher.js:170)
    at vs.convertChanges (downcastdispatcher.js:129)
    at view.change.t (editingcontroller.js:84)
    at is.change (view.js:346)
    at Ou.Ds.listenTo (editingcontroller.js:83)
    at Ou.fire (emittermixin.js:196)

Chrome points at export function toWidgetEditable( editable, writer ) { line. Can't figure out what's wrong.

Editor works without the image plugin.

webpack --mode development + uglifyjs-webpack-plugin (UglifyJs 3 - ES) plugin

Bundle size: 594 KiB - long import names, maybe related to name mangling
Bundling time: 14s

Works.

webpack --mode production + Babily plugin

Bundle size: 407 KiB
Bundling time: 60s

Works.

webpack --mode development + Babily plugin

Bundle size: 422 KiB
Bundling time: 42s

Works.

[ma2ciek]

The bug described above is introduced by UglifyJS' function inlining. It creates editable constant for https://github.com/ckeditor/ckeditor5-image/blob/00664bd1b2dec9e691a3b042078bd91219dc5e8f/src/imagecaption/utils.js#L25 and reassigned the value to another editable from https://github.com/ckeditor/ckeditor5-widget/blob/23991a4fcd3f1d24450a04877257168fbb5388f1/src/utils.js#L145 within the same scope.

Changing the variable name from editable to theEditable fixes it, so there's only one problem with that minifier. I'll report it to the UglifyJS team, as it touches some users that want to integrate CKEditor code with their own building ecosystem.

[Reinmar]

Doh, so we need to stick to babel-minify for the moment. We'll also see if this gets fixed by Uglify's team.

BTW, it reminded me about the webpack-sources's issue in Webpack that we've got in the past. It's still not fixed ;/

[ma2ciek]

Oh, I forgot to write about it. Changing minifier fixes that issue, so in that case the problem is with the babel-minify package 😞. See webpack-contrib/babel-minify-webpack-plugin#68.

[Reinmar]

So, we go with UglifyJS2 and then we have the inlining issue or with Babel minify and then we have to add the webpack-sources hack? How lovely ;>

BTW, did you check with the latest babel-preset-minify (see webpack-contrib/babel-minify-webpack-plugin#68 (comment))? Could you check it and comment under that issue if it's still occuring?

[ma2ciek]

So, we go with UglifyJS2 and then we have the inlining issue or with Babel minify and then we have to add the webpack-sources hack? How lovely ;>

Yess

BTW, did you check with the latest babel-preset-minify (see webpack-contrib/babel-minify-webpack-plugin#68 (comment))? Could you check it and comment under that issue if it's still occuring?

I'll check it, but I'm not an optimist in that case 😄

Update: It doesn't work.

[ma2ciek]

Upstream ticket for UglifyJS2: mishoo/UglifyJS#2842

[ma2ciek]

I was able to get it working by replacing optimization.minimizer in the webpack's config with a custom one.

optimization: {
	minimizer: [
		new UglifyJsWebpackPlugin( {
			sourceMap: true,
			uglifyOptions: {
				compress: {
					// Prevents inlining functions with arguments and variables.
					inline: 2
				}
			}
		} )
	]
},

With that optimization, it works with [email protected] and builts effectively (~18s and 410kB for ckeditor5-build-classic)

[ma2ciek]

There're 3 warnings visible during the building process complaining asset size, entry point size, and potential performance issues (e.g. 244KiB is the maximum size for assets). We can update these sizes to hide warnings, leave these warnings as they are or turn them off.

More info: https://webpack.js.org/configuration/performance/

[ma2ciek]

I've seen that https://github.com/webpack-contrib/extract-text-webpack-plugin, which is used by the snippet adapter is not production ready at version 4, which supports webpack@4 plugin API. It's now at the beta stage and still has some issues like webpack-contrib/extract-text-webpack-plugin#731 in the production mode. 😞

[leereichardt]

@ma2ciek

in the production mode.

Not only production mode, but also development mode 😞

[ma2ciek]

In our case somehow it works in the development mode and I've found that with BabelMinifyWebpackPlugin and optimization.minimize turned off, it works too in the production mode.

But it's something I wanted to avoid - having different minifiers and unstable packages.

[Reinmar]

Yeah... I'm surprised by the turn of events. Unlike npm for instance, webpack releases were fully functional so far. Now, we're given buggy UglifyJs2 and ecosystem which didn't catch up yet. I'm even ok with and understand the latter, but pushing UglifyJs2 as a default, without testing it thoroughly is an odd choice.

I'm afraid that the right thing to do now will be waiting. Let's just stick to webpack 3 and the current setup for the time being and see in ~1 month time. After all, we'll be back on schedule with frequent releases after this beta.

[ma2ciek]

To clarify things a bit, I've chosen UglifyJs because it's used internally by the webpack4 when setting mode: 'production'. So it can become a common choice IMO.

[Reinmar]

https://twitter.com/wSokra/status/970253245733113856

Ok here is a BETA version of a lightweight replacement of the extract-text-webpack-plugin for CSS: mini-css-extract-plugin

It uses new webpack 4 features and offers a great improvement over the ETWP:

On-Demand-Loading of separated CSS

[ma2ciek]

I'm not sure about it. It seems to be a different plugin though.

This plugin extract CSS into separate files. It creates a CSS file per JS file which contains CSS. It supports On-Demand-Loading of CSS and SourceMaps.

While we want to generate only one CSS file.

[ooflorent]

Hey folks 👋 Webpack core-team member here. I’ll be glad to assist you for this migration. Please let me know if you need assistance or advice.

[Reinmar]

Hi Florent! We'll get back to this topic soon and let you know if we have any problems. Thanks!

[ma2ciek]

I'm not sure about it. It seems to be a different plugin though.
While we want to generate only one CSS file.

I was wrong about it. I've just replaced ExtractTextWebpackPlugin with MiniCssExtractPlugin in the PR and the change was rather cosmetical. So building docs works now with webpack@4.

[ooflorent]

For webpack 4, if you want to extract CSS you must use MiniCssExtractPlugin. That's the recommendation.

[ma2ciek]

And I've added the webpack@4 as a peer dependency for ckeditor5-dev-webpack-plugin package as it breaks compatibility with the older versions.

[Reinmar]

Reminder for myself – in order to finalise this task, we'll also have to update these two docs:

• https://docs.ckeditor.com/ckeditor5/latest/framework/guides/quick-start.html
• https://docs.ckeditor.com/ckeditor5/latest/builds/guides/integration/advanced-setup.html

I had to update them recently to mention that we don't support w4 yet.

[Reinmar]

And I've added the webpack@4 as a peer dependency for ckeditor5-dev-webpack-plugin package as it breaks compatibility with the older versions.

Could you explain how it helps us? Am I right that if someone installed ckeditor5-dev-webpack-plugin but haven't installed webpack or installed wepack<4.0.0, npm will log a warning?

[Reinmar]

PS. Am I right that we're still waiting for mishoo/UglifyJS#2842?

[ma2ciek]

Could you explain how it helps us? Am I right that if someone installed ckeditor5-dev-webpack-plugin but haven't installed webpack or installed wepack<4.0.0, npm will log a warning?

Yep.

PS. Am I right that we're still waiting for mishoo/UglifyJS#2842?

No, providing option compress.inline: 2 (or 1) can disable that sort of issues.

[Reinmar]

I'm trying to understand one thing here...

Quote by @alexlamsl:

Most new projects favor uglify-es over uglify-js@3.

In which case, they are on their own - I have no plans to support harmony in the foreseeable future.

I thought that webpack 4 uses uglify-es because only this version of uglify supports ES6 (at least that's how I understood it). But you wrote previously, @ma2ciek, that webpack 4's production mode uses UglifyJs2, which, according to https://github.com/mishoo/UglifyJS2, only supports ES5.

So, I'm lost here. How does webpack 4 treat CKEditor 5's code (which is ES6) in production mode by default? With which minifier?

[ma2ciek]

Webpack4 uses https://github.com/webpack-contrib/uglifyjs-webpack-plugin/blob/master/package.json, which uses uglify-es package, which is in fact UglifyJS2 at the harmony branch.

[Reinmar]

So... webpack 4 promotes uglify-es, which according to the UglifyJs maintainer... is dead?

@ooflorent, is webpack team aware of this? I see that both issues – the specific bug (webpack-contrib/uglifyjs-webpack-plugin#264) and uglify-es not being maintained (webpack-contrib/uglifyjs-webpack-plugin#262) are tracked on your side, but there's no decision yet.

[ooflorent]

@Reinmar The webpack team is aware of this situation. Even if uglify-es is no longer maintained, it still works. There are other minification plugins for webpack. Maybe we will switch to one of them.

In webpack it is possible to choose which minifiers are applied using optimization.minizer. You could specify another one if you want to remove uglifyjs-webpack-plugin from your toolchain.

@evilebottnawi @michael-ciniawsky What are your thoughts about this?

[Reinmar]

Maybe we will switch to one of them.

I left a comment in webpack-contrib/uglifyjs-webpack-plugin#262. I guess, the topic is clear on our side – we'll need to figure out whether to use optimization.minimizer to disable inlining or to switch to babel-minify. The tricky part – i.e. making a decision about the future – is on your side :)

Thanks!

[alexander-akait]

@ooflorent I think we should migrate on babel-minify. uglify very saddened us 😞 Disable inline don't solve problem in mid/long-term, because uglify-es is very buggy 😞

[Reinmar]

@ma2ciek, there's a patch on review for uglifyjs-webpack-plugin: webpack-contrib/uglifyjs-webpack-plugin#264 (comment)

Could you test it?

[ma2ciek]

After a trouble with Lerna and rebasing old branches, I've cloned the uglifyjs-webpack-plugin to node_modules, rebuilt it and I've installed the terser package.

It looks like it's working, I've been able to remove the inline option from the UglifyJsWebpackPlugin in the webpack.config.js in ckeditor5-build-classic 🎉

[Reinmar]

BTW, what do you think about adding webpack as a peerDep of our webpack plugin? Is this common?

[ma2ciek]

Yes, it is. Look at these examples that came to my mind first:

• https://github.com/webpack-contrib/sass-loader/blob/c3482816df15efa33e7c7652fe6ce73ccc73e986/package.json#L57
• https://github.com/webpack-contrib/uglifyjs-webpack-plugin/blob/3ca5eaf7adc8f42188c29aa788c52f88b5f12464/package.json#L64
• https://github.com/babel/babel-loader/blob/1a7a4400da73d503166ca976da6eecd0cefd4774/package.json#L20

CKEditorWebpackPlugin uses the webpack's plugin API, so it needs to ensure that the correct version is used.

[Reinmar]

Cool, so let's do that together with an upgrade to w4.

[ma2ciek]

As I see, the webpack-contrib/uglifyjs-webpack-plugin#308 is merged, I checked our manual tests and they work fine with webpack@4 and [email protected].

[Reinmar]

One of the comments I wrote in the PR:

---

After a couple of deep breaths, I'm for extending CKEditorWebpackPlugin so it adds this banner in a way which doesn't conflict with minification.

This way, we'll simplify maintaining configurations. We'll be able to get rid of this:

		new webpack.BannerPlugin( {
			banner: bundler.getLicenseBanner(),
			raw: true
		} ),

And will be able to avoid adding this:

new UglifyJsWebpackPlugin( {
		sourceMap: true,
		uglifyOptions: {
			output: {
				comments: /^\**!/
			}
		}
	} )

So, CKEditorWebpackPlugin() should have the following option:

[options.addLicenseHeader=false] {Boolean|String} (or addLicenseBanner – I'm not sure which term is used out there)

• false – don't do anything
• true – add the default one
• String – if any of sub-CKEditor projects will require a bit different license text

Additionally, we may need to rethink configuration because it will get crowded:

		new CKEditorWebpackPlugin( {
			// Main language that will be built into the main bundle.
			language: 'en',

			// Additional languages that will be emitted to the `outputDirectory`.
			// This option can be set to an array of language codes or `'all'` to build all found languages.
			// The bundle is optimized for one language when this option is omitted.
			additionalLanguages: 'all',

			// Optional directory for emitted translations. Relative to the webpack's output.
			// Defaults to `'translations'`.
			// outputDirectory: 'ckeditor5-translations',

			// Whether the build process should fail if an error occurs.
			// Defaults to `false`.
			// strict: true,

			// Whether to log all warnings to the console.
			// Defaults to `false`.
			// verbose: true
		} ),

[Reinmar]

cc @pjasiun

[ma2ciek]

There's only one complication. Adding custom header we'd have to filter assets and add banner only to the ckeditor.js (or how the main build will be named) file. As for now, we don't add license headers to translations.

EDIT: I can filter them by checking if an asset has corresponding chunks, so it's not a huge problem.

[pjasiun]

I am fine with CKEditorWebpackPlugin adding a banner. However, keep in mind that it needs to handle also sourcemaps.

Also, for the internal usage, I am fine with what we have now (webpack.BannerPlugin + UglifyJsWebpackPlugin configuration). It is not that hard to understand what is going on there.

On the other hand, we should not have such code in the documentation for build CKEditor 5. The question is, if we want to have a baner in the editor build based on the documentation.

[ma2ciek]

Additionally, we may need to rethink configuration because it will get crowded:

The strict and verbose options can be merged IMO. So we can achieve a small gain from it.

I'd name the option just licenseHeader.

[Reinmar]

The strict and verbose options can be merged IMO. So we can achieve a small gain from it.

I rather meant other options. Because a group of them are language-oriented – language, additionalLanguages, outputDirectory.

---

Anyway, right now I'm having second thoughts about this change. I'm not sure whether it's better to complicate this plugin or the builds' webpack configs. Let's get back to this on Monday...

[ma2ciek]

I've checked the dedicated extractComments.banner option (I overlooked it 😞) for https://webpack.js.org/plugins/uglifyjs-webpack-plugin/, but it has its downsides too.

• It creates a *.LICENSE files with concatenated licenses from the src files, so it generates a massive duplication.
• It creates a banner, which has to follow this format: /*! ${banner} */\n (https://github.com/webpack-contrib/uglifyjs-webpack-plugin/blob/2cd4ba0e4c3d4b4531625e58277a688a7d1cfe14/src/index.js#L251)
• It complicates webpack's config a little bit

Other than that it works well.

[wizicer]

I'm facing exactly the same issue today, however, none of these advices work for me, at last, I try to separate ckeditor from uglify during webpack, here is my solution. Hope it may help someone else:

 Views/Home/Index.cshtml  | 1 +
 webpack.config.js        | 4 ++++
 webpack.config.vendor.js | 7 +++++--
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/Views/Home/Index.cshtml b/Views/Home/Index.cshtml
index 3216059..611bd5f 100644
--- a/Views/Home/Index.cshtml
+++ b/Views/Home/Index.cshtml
@@ -39,6 +39,7 @@
     GLOBAL_API_BASE_URL = "@ViewBag.ApiBaseUrl";
 </script>
 <script src="~/dist/vendor.js" asp-append-version="true"></script>
+<script src="~/dist/ckeditor.js" asp-append-version="true"></script>
 @section scripts {
     <script src="~/dist/main-client.js" asp-append-version="true"></script>
 }
diff --git a/webpack.config.js b/webpack.config.js
index b3960c8..4cb1083 100644
--- a/webpack.config.js
+++ b/webpack.config.js
@@ -35,6 +35,10 @@ module.exports = (env) => {
             new webpack.DllReferencePlugin({
                 context: __dirname,
                 manifest: require('./wwwroot/dist/vendor-manifest.json')
+            }),
+            new webpack.DllReferencePlugin({
+                context: __dirname,
+                manifest: require('./wwwroot/dist/ckeditor-manifest.json')
             })
         ].concat(isDevBuild ? [
             // Plugins that apply in development builds only
diff --git a/webpack.config.vendor.js b/webpack.config.vendor.js
index e3ea98f..c9f36e4 100644
--- a/webpack.config.vendor.js
+++ b/webpack.config.vendor.js
@@ -50,8 +50,11 @@ module.exports = (env) => {
                 'font-awesome/css/font-awesome.css',
                 'angular2-lightbox/lightbox.css',
                 'angular2-lightbox/index.js',
-                '@ckeditor/ckeditor5-build-classic/build/translations/zh-cn.js',
                 './src/styles-external.css'
+            ],
+            'ckeditor': [
+                '@ckeditor/ckeditor5-build-classic/build/translations/zh-cn.js',
+                '@ckeditor/ckeditor5-build-classic/build/ckeditor.js'
             ]
         },
         output: {
@@ -81,7 +84,7 @@ module.exports = (env) => {
                 name: '[name]_[hash]'
             })
         ].concat(isDevBuild ? [] : [
-            new webpack.optimize.UglifyJsPlugin()
+            new webpack.optimize.UglifyJsPlugin({ exclude: 'ckeditor' })
         ])
     });