T/1: Introduce CKEditor 5 builds #3

pomek · 2017-03-17T12:38:12Z

Feature: Introduced build which based on ClassicEditor. Closes ckeditor/ckeditor5#2134.

… builds.

Reinmar · 2017-03-20T10:01:59Z

build-config.js

+
+'use strict';
+
+module.exports = {


Use a real config, not a sample from the spec.

Reinmar · 2017-03-20T10:08:39Z

What I'm missing here are:

a sample,
tests (manual and automatic).

Reinmar · 2017-03-20T10:13:34Z

Also missing: .gitignore and .npmignore.

And finally, I'm curious whether the build works as a CJS module. Let's try to verify this somehow.

pomek · 2017-03-22T14:58:19Z

Ready to review.

pomek · 2017-03-22T14:58:46Z

And finally, I'm curious whether the build works as a CJS module. Let's try to verify this somehow.

Manual tests verify this.

…-utils".

Reinmar · 2017-03-24T08:51:18Z

sample/index.html

@@ -0,0 +1,175 @@
+<!DOCTYPE html>


As we talked – please use absolutely minimal HTML for now.

No images, no styling, just minimal HTML and perhaps max-width: 800px; margin: 0 auto for the body, so the editor doesn't take the whole width.

Reinmar · 2017-03-24T08:51:46Z

ckeditor.js

+		EnterPlugin,
+		HeadingPlugin,
+		ImagePlugin,
+		ImagecaptionPlugin,


ImageCaptionPlugin

It is generated by script automatically.

Ah, right. Then ok.

Reinmar · 2017-03-24T08:53:16Z

bin/build.js

+		runWebpack( webpackConfig ).then( () => log.info( 'The "ES6" build has been created.' ) ),
+		runWebpack( webpackCompactConfig ).then( () => log.info( 'The "Compact" build has been created.' ) )
+	] )
+	.then( () => {


Don't indent these lines. We write such code like this:

x( [ a, b ] ) .then( () => { c } );

Reinmar · 2017-03-24T08:57:16Z

bin/build.js

+const webpackConfig = webpackUtils.getWebpackConfig( webpackParams );
+const webpackCompactConfig = webpackUtils.getWebpackCompactConfig( webpackParams );
+
+log.info( `Creating the "ES6" and "Compact" builds...` );


Compat, not Compact ;). Remember to also fix file names.

Besides, you're not creating anything here. You're building the editor. So you can just log "Building..." and it is enough.

Reinmar · 2017-03-24T09:17:19Z

tests/ckeditor.js

+			return editor.destroy();
+		} );
+
+		it( 'should load all its dependencies', () => {


I think that we shouldn't test this. It's 1:1 with the code so whatever you change there, you change here and you don't know if it works still. Check the behaviour, not the code.

pomek · 2017-03-27T09:34:47Z

@Reinmar, ready to review.

Reinmar · 2017-03-27T09:43:13Z

bin/build.js

+const webpackConfig = webpackUtils.getWebpackConfig( webpackParams );
+const webpackCompatConfig = webpackUtils.getWebpackCompatConfig( webpackParams );
+
+log.info( `Building the "ES6" and "Compat" editors...` );


As I wrote – KISS, just "Building..." will be fine.

Reinmar · 2017-03-27T09:44:33Z

bin/build.js

+log.info( `Building the "ES6" and "Compat" editors...` );
+
+Promise.all( [
+	runWebpack( webpackConfig ).then( () => log.info( 'The "ES6" editor has been built.' ) ),


Finished building "build/ckeditor.js".
Finished building "build/ckeditor.compat.js".

…he second - for the editor.

ma2ciek · 2017-03-30T23:07:14Z

sample/index.html

-		.catch( err => {
-			console.error( err.stack );
-		} );
+	.then( editor => {


Is that a rule? I was pretty sure that if the code before a method is one-line long, then we're starting the next lines with indentation.

Yes, we rather write it this way.

I wasn't right. I thought about such a case:

ClassicEditor.create( document.querySelector( '#editor' ), { plugins: [ EssentialsPreset, Paragraph, Bold, Italic, Heading ], toolbar: [ 'headings', 'bold', 'italic', 'undo', 'redo' ] } ) .then( editor => { window.editor = editor; } ) .catch( err => { console.error( err.stack ); } );

But here we have } ) ending with indent 0, so that's why we continue with such an indentation.

In this case where .then() follows an expression which finished in one line we should rather indent them, just like @pomek did.

Ok, thanks for clarifying this.

… module resolution configured - we can use the defaults.

…hem.

Reinmar · 2017-04-03T14:44:11Z

I've checked that in order to run this package's tests on Travis we'd need a couple of new files and a lot of configuration linters. So let's live with those tests being mainly run from ckeditor5 (e.g. after commits to master-revisions). This will be enough for now.

Also, I think that builds might actually be tested differently. In the manual tests we are using the builds themselves but in the automated tests we use the source modules. It'd be better if automated tests also used the bundled modules but that would be super slow, so... yeah – we'd need to figure something out.

Anyway, this is enough for now.

Kamil Piechaczek added 6 commits March 17, 2017 13:36

Initial commit.

24486ff

Fixed invalid file names.

10aec02

Added missing package to package.json.

74a7c82

Introduced "@ckeditor/ckeditor5-dev-bundler-webpack" for creating the…

dc33347

… builds.

Fixed variable names.

92c8d62

Added build for the latest changes in all packages.

6d1bc20

pomek requested a review from Reinmar March 17, 2017 12:38

Reinmar reviewed Mar 20, 2017

View reviewed changes

Reinmar added the status:review- label Mar 20, 2017

Kamil Piechaczek added 9 commits March 21, 2017 11:46

Added dot files.

1c94ca1

Added real plugins to the build.

dd96494

Adjusted build script to the latest changes with dev packages.

57142ff

Tests: Added test for generated editor.

4a1d6f7

Tests: Added manual tests.

b3beaee

Introduced sample.

3efa8c5

Added "autoformat" plugin; some improvements in builds.

a232d7b

Update the builds.

5be7d09

Introduced Compact build, ES6 build is now default build.

6c5674e

pomek removed the status:review- label Mar 22, 2017

Kamil Piechaczek added 2 commits March 22, 2017 16:14

Adjusted build script to the latest changes in "ckeditor5-dev-webpack…

d145b35

…-utils".

Webpack's utils do not require the 'moduleName' option.

15b870c

Reinmar reviewed Mar 24, 2017

View reviewed changes

Reinmar added the status:review- label Mar 27, 2017

Kamil Piechaczek added 2 commits March 27, 2017 11:18

Adjusted scripts to the latest changes in "ckeditor5-dev-webpack-utils".

5e065e3

Tests: Improved automated test.

888a7b1

pomek removed the status:review- label Mar 27, 2017

Tests: Minor changes in HTML.

a736e80

Reinmar reviewed Mar 27, 2017

View reviewed changes

Kamil Piechaczek and others added 5 commits March 27, 2017 11:47

Simplified the logs.

5caac8c

Split config to two files. The first contains parameters for build, t…

f85913f

…he second - for the editor.

The latest build.

fb30967

Tests: Renamed manual tests (used a period instead of a dash). [skip ci]

fa756a8

Various improvements.

281027a

ma2ciek reviewed Mar 30, 2017

View reviewed changes

Reinmar added 4 commits March 31, 2017 10:12

Minor improvement in tests.

d46781d

The cwd option was removed because the configs don't have to have the…

eb2ce07

… module resolution configured - we can use the defaults.

Moved Webpack configs to this repository for simpler tinkering with t…

e9b734b

…hem.

Final cleanup.

64ca3c9

Reinmar force-pushed the t/1 branch from 4ce7b94 to 64ca3c9 Compare April 3, 2017 14:37

Note in the manual tests that they need the bundles to be rebuilt.

f280a8b

Reinmar merged commit f88bc52 into master Apr 3, 2017

Reinmar deleted the t/1 branch April 3, 2017 14:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T/1: Introduce CKEditor 5 builds #3

T/1: Introduce CKEditor 5 builds #3

pomek commented Mar 17, 2017 •

edited

Loading

Reinmar Mar 20, 2017

Reinmar commented Mar 20, 2017 •

edited

Loading

Reinmar commented Mar 20, 2017

pomek commented Mar 22, 2017

pomek commented Mar 22, 2017

Reinmar Mar 24, 2017

Reinmar Mar 24, 2017

Reinmar Mar 24, 2017

pomek Mar 24, 2017

Reinmar Mar 24, 2017

Reinmar Mar 24, 2017

Reinmar Mar 24, 2017

Reinmar Mar 24, 2017 •

edited

Loading

Reinmar Mar 24, 2017

pomek commented Mar 27, 2017

Reinmar Mar 27, 2017

Reinmar Mar 27, 2017

ma2ciek Mar 30, 2017

Reinmar Mar 31, 2017

Reinmar Mar 31, 2017

ma2ciek Mar 31, 2017

Reinmar commented Apr 3, 2017


		'use strict';

		module.exports = {

T/1: Introduce CKEditor 5 builds #3

T/1: Introduce CKEditor 5 builds #3

Conversation

pomek commented Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Reinmar commented Mar 20, 2017 • edited Loading

Reinmar commented Mar 20, 2017

pomek commented Mar 22, 2017

pomek commented Mar 22, 2017

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

Reinmar Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pomek commented Mar 27, 2017

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

Reinmar commented Apr 3, 2017

pomek commented Mar 17, 2017 •

edited

Loading

Reinmar commented Mar 20, 2017 •

edited

Loading

Reinmar Mar 24, 2017 •

edited

Loading