T/131: Introduce CKEditor 5 builds #132

pomek · 2017-03-16T13:43:51Z

Feature: Added a util for creating an entry point for Rollup/Webpack. Introduced a new editor bundler - ckeditor5-dev-bundler-webpack. Closes #131.

…5-dev-bundler-rollup". Removed existing util which did it and its related code.

Reinmar · 2017-03-20T09:45:51Z

packages/ckeditor5-dev-bundler-webpack/README.md

+export default class ClassicEditor extends ClassicEditorBase {}
+
+ClassicEditor.build = {
+	plugins: [ ArticlePlugin ],


It's ArticlePreset.

Reinmar · 2017-03-20T09:46:12Z

packages/ckeditor5-dev-bundler-webpack/lib/tasks/runwebpack.js

+ * @param {Object} config
+ * @returns {Promise}
+ */
+module.exports = function runWebpack( config ) {


Why do we need this module?

As we talked F2F, we don't need this module. I'll remove it.

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

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

+    "babel-core": "^6.24.0",
+    "babel-loader": "^6.4.0",
+    "babel-preset-env": "^1.2.2",
+    "babel-preset-es2015": "^6.24.0",


Don't use this (see ckeditor/ckeditor5#415).

Reinmar · 2017-03-20T10:04:14Z

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

+  "dependencies": {
+    "babel-core": "^6.24.0",
+    "babel-loader": "^6.4.0",
+    "babel-preset-env": "^1.2.2",


Switch the code to use this preset.

Reinmar · 2017-03-20T10:07:53Z

packages/ckeditor5-dev-bundler-webpack/lib/utils/getwebpackes6config.js

+
+		output: {
+			path: options.destinationPath,
+			filename: 'ckeditor.es6.js',


We redefined a bit the two types of builds: ckeditor/ckeditor5#408 (comment)

Reinmar · 2017-03-20T10:17:15Z

packages/ckeditor5-dev-bundler-webpack/lib/utils/getwebpackconfig.js

+ * @returns {Object}
+ */
+module.exports = function getWebpackConfig( options ) {
+	const config = getWebpackES6Config( options );


I don't like that one config is an extension of the other one because it's not. See that you're overriding the plugins property, so it's not an inheritance and it shouldn't be implemented here as such.

E.g. what if the ES6 config will require more plugins? You don't know if you need to override just Babili or all of them.

So, let's KISS here and just duplicate the config object twice. After all – "Duplication is easier to fix than broken abstraction"

Reinmar · 2017-03-20T10:18:00Z

packages/ckeditor5-dev-bundler-webpack/lib/utils/getwebpackes6config.js

+
+		resolveLoader: {
+			modules: [
+				path.join( options.cwd, 'node_modules' )


I don't think this is needed. Or at least – it should not be needed.

Reinmar · 2017-03-20T10:18:20Z

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

+    "babel-loader": "^6.4.0",
+    "babel-preset-env": "^1.2.2",
+    "babel-preset-es2015": "^6.24.0",
+    "babili-webpack-plugin": "0.0.11",


Reinmar · 2017-03-20T10:19:09Z

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

+	const plugins = getPlugins( options.plugins );
+
+	let content = `/**
+ * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.


Don't hardcode 2017.

Reinmar · 2017-03-20T10:19:33Z

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

+ * @param {Object} config
+ * @returns {String}
+ */
+module.exports = function getEditorConfig( config ) {


What is this function doing?

It makes string from specified config. I understand you are talking about missing JSDoc.

Reinmar · 2017-03-20T10:19:54Z

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

+ * @param {Array.<String>} pluginPaths
+ * @returns {Object}
+ */
+module.exports = function getPlugins( pluginPaths ) {


What is this function doing?

Missing JSDoc, right?

I'm afraid it's not only that. But I'll wait for the docs to verify this ;)

… 'dev-webpack-utils'.

Read more: webpack/webpack#3929.

pomek · 2017-03-22T12:55:57Z

No, it isn't ready. I forgot to implement ckeditor/ckeditor5#408 (comment) (and comments below).

pomek · 2017-03-22T14:51:28Z

Ok, should be fine.

… "ckeditor5-dev-webpack-utils".

Reinmar · 2017-03-24T09:46:02Z

packages/ckeditor5-dev-utils/tests/bundler/createentryfile.js

+			} );
+
+			const expectedEntryFile = `/**
+ * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.


Imagine it's January 1, 2018 ;)

Sure :D

Will fix it.

Reinmar · 2017-03-24T10:06:25Z

🍆

Reinmar · 2017-03-24T10:10:56Z

I noticed that you've moved Webpack plugin to the new package. I'd keep it in the old one. It'll be used far more often and by far more developers than the utils, so let's expose it better.

pomek · 2017-03-27T08:30:15Z

@Reinmar, ready to review once again.

…enamed as "dev-webpack-utils". File: packages/ckeditor5-dev-tests/package.json. This reverts commit 6747044.

Reinmar · 2017-03-27T09:41:49Z

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

+ * @param {Object} config
+ * @returns {String}
+ */
+module.exports = function getEditorConfig( config ) {


This isn't very safe, unfortunately. E.g. every " character will be replaced with ' which certainly isn't correct.

Therefore, let's do the simplest possible thing now – output a JSON string to the entry file and parse it there. The entry file should look like this:

const config = JSON.parse( '...' );

Or, to keep the config more readable in the entry file:

const config = JSON.parse( `{ "foo": "bar" }` );

Ooor... we can keep the editor config in a separate file (editor-config.js) – next to ckeditor.js and simply import it. Then, we won't need to stringify anything. You could check this option first.

Will check it. Thanks for the idea.

Since the config will be a separate file, the createEntryPoint() function could get its path as an argument.

Reinmar · 2017-03-27T09:57:02Z

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

+
+	pluginPaths.forEach( ( pathToFile ) => {
+		const basePluginName = capitalize( path.basename( pathToFile, '.js' ) );
+		let pluginName = basePluginName + 'Plugin';


Something's seriously wrong in here. Why isn't this a const? Why there's a loop which uses the same variable name in the loop and then assigns a new value to it?

If this code works, then it needs to be refactored to also have any sense ;) I looked at it for 10s and don't get it which means that it's wrong :P

This code has sense and works - it generates a unique name for a plugin.

If given name is already used, the next one will contain a number at the end.

Why isn't this a const?

Because: pluginName = basePluginName + ( ++i ).toString() + 'Plugin'; in the while loop.

It means, given plugin name is already used. We need to generate new.

OTOH, maybe it is not required because plugins with the same names shouldn't exist.

I looked at it for 10s and don't get it which means that it's wrong :P

Sorry, but does not sound like a metric for me. ;/

And the code is already in use - https://github.com/ckeditor/ckeditor5-dev/blob/master/packages/ckeditor5-dev-bundler-rollup/lib/utils.js#L123-L149.

Sorry, but does not sound like a metric for me. ;/

Well, this is the most important metric of a good code. If it's understandable to other developers.

…function expects a path to an external editor configuration file.

… of being an option.

…s now).

…ect because it was used incorrectly. Once we really turn it on we also need to add a banner with the license.

…lts.

Reinmar · 2017-04-03T11:38:43Z

I've tried to play a bit with ckeditor5-build-classic and I realised that it's very cumbersome because there are no webpack configs there to play with. So I think that it'll be better to actually keep them in the build repos. This means that they'll be duplicated and a bit harder to maintain but the DX will be better.

PS. And the thing I've been trying to check is removePlugins option... and it doesn't work.

Reinmar · 2017-04-03T12:41:04Z

Another thing I noticed is that the way how we run Webpack swallows errors... So, I'm gonna remove or simplify bin/build.js and run Webpack directly.

Kamil Piechaczek added 4 commits March 16, 2017 11:30

Feature: Introduced a script which creates an entry file for bundler.

84640ff

Tests: Initial tests for bundler functions.

a3a3084

Feature: Introduced utils for generating an entry file into "ckeditor…

597a041

…5-dev-bundler-rollup". Removed existing util which did it and its related code.

Initial commit for "@ckeditor/ckeditor5-dev-bundler-webpack".

675ad25

pomek added the status:review- label Mar 16, 2017

Kamil Piechaczek added 2 commits March 17, 2017 09:59

Tests: Added tests for function which create an entry file.

5dde142

Tests: Added tests for ckeditor5-dev-bundler-webpack.

cb4b621

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

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

pomek changed the title ~~[WIP] T/131: Introduce CKEditor 5 builds~~ T/131: Introduce CKEditor 5 builds Mar 17, 2017

pomek mentioned this pull request Mar 17, 2017

T/1: Introduce CKEditor 5 builds ckeditor/ckeditor5-build-classic#3

Merged

Reinmar reviewed Mar 20, 2017

View reviewed changes

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

Kamil Piechaczek added 7 commits March 20, 2017 11:51

Merged 'dev-bundler-webpack' into 'dev-webpack-plugin' and renamed as…

6747044

… 'dev-webpack-utils'.

Fixed indent in generated list of plugins in an entry file.

4eea8b2

Fixed failed test.

63aa8ec

Docs: Added documentation to new functions.

9cb6ec6

Webpack - used preset "env" instead of "es2015".

878724b

Merge branch 'master' into t/131

ac8e956

Fixed invalid UMD build.

d80bdae

Read more: webpack/webpack#3929.

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

Renamed configuration files.

0fa69c4

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

Fixed invalid names of functions exported by the main file in package…

6ea012c

… "ckeditor5-dev-webpack-utils".

Reinmar reviewed Mar 24, 2017

View reviewed changes

Kamil Piechaczek added 2 commits March 27, 2017 10:29

Seperated Webpack plugin and Webpack utils.

20d1ea2

Tests: Fixed hardcoded year in the tests.

a7f1958

Kamil Piechaczek added 2 commits March 27, 2017 10:31

Revert: 'Merged "dev-bundler-webpack" into "dev-webpack-plugin" and r…

8c7249f

…enamed as "dev-webpack-utils". File: packages/ckeditor5-dev-tests/package.json. This reverts commit 6747044.

Webpack-utils: fixed invalid name exported by compat config.

56761b4

Reinmar reviewed Mar 27, 2017

View reviewed changes

Kamil Piechaczek and others added 9 commits March 27, 2017 12:19

Removed "getEditorConfig" function. Instead of it, "createEntryFile" …

b1639a5

…function expects a path to an external editor configuration file.

PA pth to an external editor configuration is now a parameter instead…

afe979d

… of being an option.

Merge branch 'master' into t/131

71fcb86

Removed compiler utils (not used for a long time, but let's clean thi…

fcdff80

…s now).

Various minor fixes.

3a929bc

Fixed filenames in webpack-plugin.

ad97550

Fixed license banners – the comments:false used previously had no eff…

a695ac5

…ect because it was used incorrectly. Once we really turn it on we also need to add a banner with the license.

Remove cwd option and module resolution setting – let's use the defau…

5535ac0

…lts.

Added info about the new package to the main readme.

5da0437

Moved configs to the build repos.

90e1459

Reinmar merged commit 0aa523b into master Apr 3, 2017

Reinmar deleted the t/131 branch April 3, 2017 14:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T/131: Introduce CKEditor 5 builds #132

T/131: Introduce CKEditor 5 builds #132

pomek commented Mar 16, 2017 •

edited

Loading

Reinmar Mar 20, 2017

Reinmar Mar 20, 2017

pomek Mar 20, 2017

Reinmar Mar 20, 2017

Reinmar Mar 20, 2017

Reinmar Mar 20, 2017

Reinmar Mar 20, 2017

Reinmar Mar 20, 2017

Reinmar Mar 20, 2017

Reinmar Mar 20, 2017

Reinmar Mar 20, 2017

pomek Mar 20, 2017

Reinmar Mar 20, 2017

pomek Mar 20, 2017

Reinmar Mar 20, 2017

pomek commented Mar 22, 2017 •

edited

Loading

pomek commented Mar 22, 2017

Reinmar Mar 24, 2017

pomek Mar 24, 2017

Reinmar commented Mar 24, 2017 •

edited

Loading

Reinmar commented Mar 24, 2017

pomek commented Mar 27, 2017

Reinmar Mar 27, 2017 •

edited

Loading

pomek Mar 27, 2017

Reinmar Mar 27, 2017

Reinmar Mar 27, 2017

pomek Mar 27, 2017

pomek Mar 27, 2017

Reinmar Mar 31, 2017

Reinmar commented Apr 3, 2017

Reinmar commented Apr 3, 2017

T/131: Introduce CKEditor 5 builds #132

T/131: Introduce CKEditor 5 builds #132

Conversation

pomek commented Mar 16, 2017 • edited Loading

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

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

pomek commented Mar 22, 2017 • edited Loading

pomek commented Mar 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar commented Mar 24, 2017 • edited Loading

Reinmar commented Mar 24, 2017

pomek commented Mar 27, 2017

Reinmar Mar 27, 2017 • edited Loading

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 commented Apr 3, 2017

Reinmar commented Apr 3, 2017

pomek commented Mar 16, 2017 •

edited

Loading

pomek commented Mar 22, 2017 •

edited

Loading

Reinmar commented Mar 24, 2017 •

edited

Loading

Reinmar Mar 27, 2017 •

edited

Loading