Upgrade webpack plugin and testing environment for webpack v4 #373

ma2ciek · 2018-03-06T14:30:38Z

Suggested merge commit message (convention)

Other: Updated CKEditorWebpackPlugin to support webpack@4. Updated webpack in testing tools. Closes #371.

Additional information

Changes in other repos:

BREAKING CHANGE: Breaks compatibility with webpack v3.

ma2ciek · 2018-03-06T14:59:29Z

I just saw that one test, which uses import * as enitreModule from 'someModule' fails during creating a spy. https://github.com/ckeditor/ckeditor5-ui/blob/a98b63ef0b7995db9fde3f2296e5fddcadb36a5d/tests/panel/balloon/balloonpanelview.js#L175

ma2ciek · 2018-03-06T17:39:39Z

Yep, now webpack creates only getters for module exports, so sinon can't wrap these functions. We can drop this test or attach getOptimalPosition to the BalloonPanelView, to enable wrapping it.

cc @oleq, @Reinmar

coveralls · 2018-03-22T14:09:13Z

Coverage decreased (-0.1%) to 90.188% when pulling f64d160 on t/371 into 404bdec on master.

…s` as it is not used.

…nts options.

ma2ciek · 2018-06-28T15:33:18Z

$ lerna bootstrap
lerna info version 2.11.0
lerna info versioning independent
lerna info Bootstrapping 8 packages
lerna info lifecycle preinstall
lerna info Installing external dependencies
lerna info hoist Installing hoisted dependencies into root

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

Not sure whether it's too low timeout set for Lerna 🤔

ma2ciek · 2018-06-28T15:37:17Z

packages/ckeditor5-dev-utils/lib/bundler/getlicensebanner.js

@@ -10,7 +10,7 @@ module.exports = function getLicenseBanner() {

 	/* eslint-disable indent */
 	return (
-`/**
+`/*!


I changed comment, so it differs now a little bit from the original comments used in the code. It's the result of webpack/webpack#6630 (comment).

Can we use /* (with one *)? This ! looks like a typo.

Or something like:

/*-------------------------* * * *-------------------------*/

Anything what does not look like a typo.

I'm ok with

/*-------------------------* * * *-------------------------*/

It just needs to differ from original license headers.

pomek · 2018-06-29T09:01:13Z

Not sure whether it's too low timeout set for Lerna 🤔

Travis has some troubles.

Reinmar · 2018-06-29T12:05:47Z

packages/ckeditor5-dev-utils/lib/bundler/getlicensebanner.js

- * @license Copyright (c) 2003-${ date.getFullYear() }, CKSource - Frederico Knabben. All rights reserved.
- * For licensing, see LICENSE.md.
- */`
+`/*--------------------------------------------------------------------------------------*


It doesn't seem like nicely aligned. Is it GH's way of rendering this string? Or isn't the box wide enough?

${ date.getFullYear() } that becomes 4-character-long as it evaluates.

Reinmar · 2018-06-29T12:07:48Z

packages/ckeditor5-dev-utils/lib/bundler/getlicensebanner.js

- * @license Copyright (c) 2003-${ date.getFullYear() }, CKSource - Frederico Knabben. All rights reserved.
- * For licensing, see LICENSE.md.
- */`
+`/*--------------------------------------------------------------------------------------*


It requires a comment on why it needs to look this way.

We can think about a shorter comment or some short custom plugin for the banner.

BTW can't we look for /\/\*\n \* @license/? And use the old license comment?

Because once you wrote about a shorter comment I thought that it unnecessarily increases a size of every build. Not by much, perhaps it gzips well, but it seems unnecessary if the regexp can look for @license too.

Doh, we use @license in every single source file :D

See #373 (comment) too 😄

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.addLicense=false] {Boolean|String}

false – don't add

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 } ),

I'll move this to the ticket cause it'd be pity to lose this.

ma2ciek · 2018-07-03T15:27:36Z

Ready for the review once again :)

Reinmar · 2018-07-05T10:45:15Z

package.json

@@ -33,7 +33,8 @@
    ]
  },
  "eslintIgnore": [
-    "coverage/**"
+	"coverage/**",
+	"packages/*/node_modules/**"


When installing two versions of dependencies in 2 different packages inside ckeditor5-dev's subrepos, both aren't moved to the ckeditor5-dev by Lerna.

Reinmar · 2018-07-05T10:51:08Z

packages/ckeditor5-dev-webpack-plugin/lib/servetranslations.js

-	compiler.plugin( 'after-resolvers', () => {
-		const resolver = compiler.resolvers.normal;
+	// Add core translations before `translateSourceLoader` starts translating.
+	compiler.hooks.normalModuleFactory.tap( 'CKEditor5Plugin', normalModuleFactory => {


Is this consistent with webpack plugins' naming convention?

Yep, see https://github.com/webpack/tapable.

Reinmar · 2018-07-05T10:52:08Z

packages/ckeditor5-dev-webpack-plugin/package.json

@@ -6,7 +6,12 @@
  "main": "lib/index.js",
  "dependencies": {
    "@ckeditor/ckeditor5-dev-utils": "^9.0.1",
-    "rimraf": "^2.6.2"
+    "chalk": "^2.4.1",


Is it imported anywhere?

https://github.com/ckeditor/ckeditor5-dev/pull/373/files/127cd77aa4d7d048b125896ef8448ce31443ad63#diff-088f1ffe6ad43d8c12490e6e14c9c73bR8

Reinmar · 2018-07-05T10:52:17Z

packages/ckeditor5-dev-webpack-plugin/package.json

-    "rimraf": "^2.6.2"
+    "chalk": "^2.4.1",
+    "rimraf": "^2.6.2",
+    "webpack-sources": "^1.0.0"


And this too?

I actually see it was imported previously. So it was missing previously, yes?

https://github.com/ckeditor/ckeditor5-dev/pull/373/files/127cd77aa4d7d048b125896ef8448ce31443ad63#diff-088f1ffe6ad43d8c12490e6e14c9c73bL12

Both were missing.

ma2ciek changed the title ~~Upgrade webpack plugin and testing environment for webpack v4~~ [Do not merge] Upgrade webpack plugin and testing environment for webpack v4 Mar 7, 2018

ma2ciek force-pushed the t/371 branch from f6907c8 to 9128b76 Compare March 22, 2018 14:17

pomek mentioned this pull request Mar 27, 2018

Upgraded the dependencies #390

Merged

ma2ciek force-pushed the t/371 branch from 6bcc4ca to 26cc9e6 Compare June 4, 2018 14:22

ma2ciek force-pushed the t/371 branch from 26cc9e6 to db5bc20 Compare June 26, 2018 09:01

ma2ciek added 8 commits June 28, 2018 16:35

Added support for webpack@4.

a4800e1

Changed webpack plugins into hooks.

f72f097

Added webpack@4 to testing utils.

ca38f70

Upgraded webpack version to 4.2.0.

bb17495

Added webpack as a peer dependency.

727467c

Removed CKEditor5WebpackPlugin from the `@ckeditor/ckeditor5-dev-test…

ce9a914

…s` as it is not used.

Improved banner license, so it can be used easily with uglifyjs comme…

feb660d

…nts options.

Fixed conflict with banner.

3da1e9d

ma2ciek force-pushed the t/371 branch from e7bf068 to 3da1e9d Compare June 28, 2018 14:36

ma2ciek added the status:review- label Jun 28, 2018

ma2ciek commented Jun 28, 2018

View reviewed changes

ma2ciek changed the title ~~[Do not merge] Upgrade webpack plugin and testing environment for webpack v4~~ Upgrade webpack plugin and testing environment for webpack v4 Jun 29, 2018

Updated license banner.

ff8fe70

Reinmar reviewed Jun 29, 2018

View reviewed changes

ma2ciek mentioned this pull request Jun 29, 2018

Fix test, so it passes with webpack@4. ckeditor/ckeditor5-ui#421

Merged

ma2ciek added 4 commits July 3, 2018 13:32

Switched back to banner with exclamation mark.

1bbdec9

Fixed license banner test.

46a07e1

Added comment for the special banner.

0c7e856

Updated chalk package.

13e3941

Improved linter.

127cd77

ma2ciek removed the status:review- label Jul 3, 2018

Reinmar reviewed Jul 5, 2018

View reviewed changes

Reinmar added 2 commits July 5, 2018 12:57

Merge branch 'master' into t/371

b21a87d

Bump, bump, bump, bump.

f64d160

Reinmar merged commit d0cbbca into master Jul 5, 2018

Reinmar deleted the t/371 branch July 5, 2018 12:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade webpack plugin and testing environment for webpack v4 #373

Upgrade webpack plugin and testing environment for webpack v4 #373

ma2ciek commented Mar 6, 2018 •

edited

Loading

ma2ciek commented Mar 6, 2018

ma2ciek commented Mar 6, 2018

coveralls commented Mar 22, 2018 •

edited

Loading

ma2ciek commented Jun 28, 2018

ma2ciek Jun 28, 2018

pjasiun Jun 29, 2018

pjasiun Jun 29, 2018

ma2ciek Jun 29, 2018

pomek commented Jun 29, 2018

Reinmar Jun 29, 2018

ma2ciek Jun 29, 2018

Reinmar Jun 29, 2018

ma2ciek Jun 29, 2018

Reinmar Jun 29, 2018

Reinmar Jun 29, 2018

Reinmar Jun 29, 2018

ma2ciek Jun 29, 2018

Reinmar Jun 29, 2018

Reinmar Jun 29, 2018

ma2ciek commented Jul 3, 2018

Reinmar Jul 5, 2018

ma2ciek Jul 5, 2018

Reinmar Jul 5, 2018

ma2ciek Jul 5, 2018

Reinmar Jul 5, 2018

ma2ciek Jul 5, 2018

Reinmar Jul 5, 2018

Reinmar Jul 5, 2018

ma2ciek Jul 5, 2018

ma2ciek Jul 5, 2018

Upgrade webpack plugin and testing environment for webpack v4 #373

Upgrade webpack plugin and testing environment for webpack v4 #373

Conversation

ma2ciek commented Mar 6, 2018 • edited Loading

Suggested merge commit message (convention)

Additional information

ma2ciek commented Mar 6, 2018

ma2ciek commented Mar 6, 2018

coveralls commented Mar 22, 2018 • edited Loading

ma2ciek commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pomek commented Jun 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ma2ciek commented Jul 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ma2ciek commented Mar 6, 2018 •

edited

Loading

coveralls commented Mar 22, 2018 •

edited

Loading