T/49 #70

pomek · 2017-02-28T08:14:52Z

Suggested merge commit message (convention)

Feature: PluginCollection - support for loading plugin by names. Closes ckeditor/ckeditor5#2873.

Introduced a new parameter for PluginCollection.load() - removePlugins.
The parameter is a list which defines plugins that won't be loaded.

Additional information

…gin names.

…movePlugins" option.

Reinmar · 2017-02-28T08:17:13Z

src/editor/editor.js

@@ -45,7 +45,7 @@ export default class Editor {
 		 * @readonly
 		 * @member {module:core/plugin~PluginCollection}
 		 */
-		this.plugins = new PluginCollection( this );
+		this.plugins = new PluginCollection( this, this.config.get( 'plugins' ) );


This is not according to the spec (https://github.com/ckeditor/ckeditor5-core/issues/49#issuecomment-282299034).

available plugins !== config.plugins

There are no available plugins build in the editor yet because it's a separate ticket: https://github.com/ckeditor/ckeditor5-core/issues/67. So you can revert this change.

Reinmar · 2017-02-28T08:18:13Z

src/plugincollection.js

+
+		// Save available plugins.
+		for ( const plugin of availablePlugins ) {
+			this._availablePlugins.set( plugin.pluginName, plugin );


Plugin name is optional.

Reinmar · 2017-02-28T08:32:12Z

src/plugincollection.js

 				.catch( ( err ) => {
 					/**
 					 * It was not possible to load the plugin.
 					 *
 					 * @error plugincollection-load
 					 * @param {String} plugin The name of the plugin that could not be loaded.
 					 */
-					log.error( 'plugincollection-load: It was not possible to load the plugin.', { plugin: PluginConstructor } );
+					log.error( 'plugincollection-load: It was not possible to load the plugin.', { plugin: PluginConstructorOrName } );


Do I see correctly that this error will be thrown if a plugin cannot be load because it cannot be found by its name? This should be a separate, more meaningful error.

https://github.com/ckeditor/ckeditor5-core/blob/master/src/plugincollection.js#L89

Reinmar · 2017-02-28T08:33:11Z

src/plugincollection.js

+							 * @param {*} plugin The dependency constructor which is meant to be loaded as a plugin.
+							 * @param {*} requiredBy The parent constructor which is meant to be loaded as a plugin.
+							 */
+							// jscs:disable maximumLineLength


Don't disable the linter. Just fix the code (split the text).

Reinmar · 2017-02-28T10:19:38Z

src/plugincollection.js

@@ -29,9 +30,24 @@ export default class PluginCollection {

 		/**
 		 * @protected
+		 * @member {Map.<String,module:core/plugin~Plugin>} module:core/plugin~PluginCollection#_availablePlugins


The key may not be a string AFAICS below. It may be a function too.

Reinmar · 2017-02-28T10:22:30Z

src/plugincollection.js

@@ -110,6 +166,29 @@ export default class PluginCollection {
 			} );
 		}

+		function getPluginConstructor( PluginConstructorOrName ) {
+			if ( typeof PluginConstructorOrName === 'function' ) {


We use == after typeof.

…lection.

…ng the `typeof` results.

Reinmar · 2017-02-28T10:51:22Z

src/plugincollection.js

+
+			if ( !PluginConstructor ) {
+				/**
+				 * The loaded plugin module is not available.


Update to:

The plugin cannot be loaded by name. Plugin classes need to be provided to the editor before they can be loaded by name. This is usually done by the builder. TODO update this error with links to docs because it will be a very frequent problem. @error plugincollection-plugin-not-found @param {String} pluginName The name of the plugin which could not be loaded.

Reinmar · 2017-02-28T12:46:31Z

OK, I checked the code more carefully and there are couple of things which should be refined here.

The implementation became too complex now, mostly because unification of what kind of plugin (string or constructor) was passed to load() is done too late.

The plugins array should be turned into an array of constructors ASAP. This way you'll avoid having PluginConstructor__OrName__ all around that code.
There should be no error if you executed load( [ PluginA ], [ 'NonExistentPlugin' ] ). The error which would be thrown now makes no sense in this case. We can simply ignore names for which we can't match any plugins.
Don't modify the removePlugins variable in load(). Create a new one which will better tell what it is. Also, from what I can see it can be just an array of constructors. It doesn't need to contain anything more.

Besides, you had a missing code coverage (which I fixed in my last commit).

pomek · 2017-03-01T09:23:39Z

Let's try once again :D

Reinmar · 2017-03-01T09:27:00Z

src/plugincollection.js

+			.map( ( item ) => getPluginConstructor( item ) )
+			.filter( ( PluginConstructor ) => !!PluginConstructor );
+
+		// Check whether a length of passed array with plugins is the same like an array with mapped plugins.


As you noticed yourself, this comment needs to be refined :D

Besides, I don't like splitting the filtering which is done above from the check which is done below because it's a single piece of logic – can't find a plugin – throw.

OTOH, I can see that when using filter() it would be hard to reject the promise and stop the execution, so the check makes sense to be separated. So let's keep it like that, but please move it to a separate function. If you'll name this function intuitively, you won't need that comment :P.

The code in here could look like this:

const missingPlugins = getMissingPlugins( plugins, pluginConstructorsToLoad ); if ( missingPlugins ) { return Promise.reject(); }

Reinmar · 2017-03-01T09:39:57Z

src/plugincollection.js

+		// Check whether a length of passed array with plugins is the same like an array with mapped plugins.
+		if ( pluginConstructorsToLoad.length !== plugins.length ) {
+			// Extract names of plugins which cannot be loaded.
+			const notFoundPlugins = plugins.filter( ( item ) => !pluginConstructorsToLoad.includes( item ) );


I think that this is a wrong way of getting missing plugins. In plugins you may have strings and in pluginConstructorsToLoad you will have constructors. So checking if the former is in the latter doesn't make sense. Use getPluginConstructor() again.

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

src/plugincollection.js

+					PluginConstructor.requires.forEach( ( RequiredPluginConstructorOrName ) => {
+						const RequiredPluginConstructor = getPluginConstructor( RequiredPluginConstructorOrName );
+
+						if ( !RequiredPluginConstructor ) {


This is unnecessary in this PR. It doesn't make sense for a to require other plugins by their names.

PS. I meant that this may be a possible feature in the future, but it's not part of the ticket for which you're making a PR, so it shouldn't be included in it.

Reinmar · 2017-03-01T09:42:37Z

src/plugincollection.js

 			.then( () => loaded );

-		function loadPlugin( PluginConstructorOrName ) {
+		function loadPlugin( PluginConstructor ) {
 			// Don't load the plugin if it cannot be loaded.


This comment is unnecessary.

…be loaded (cause they are not defined).

…exist.

Reinmar · 2017-03-01T17:59:10Z

src/plugincollection.js

 	 *
 	 * @param {module:core/editor/editor~Editor} editor
+	 * @param {Array.<Function>} availablePlugins Plugins (constructor) which the collection will be able to use


This is optional.

Reinmar · 2017-03-01T18:00:44Z

src/plugincollection.js

 	 * @returns {Promise} A promise which gets resolved once all plugins are loaded and available into the
 	 * collection.
-	 * @param {Array.<module:core/plugin~Plugin>} returns.loadedPlugins The array of loaded plugins.
+	 * @returns {Promise.<Array.<module:core/plugin~Plugin>>} returns.loadedPlugins The array of loaded plugins.


This is optional.

Reinmar · 2017-03-01T18:04:54Z

src/plugincollection.js

+		const missingPlugins = getMissingPlugins( plugins );
+
+		if ( missingPlugins ) {
+			log.error( 'plugincollection-load: It was not possible to load the plugins.', { plugins: missingPlugins } );


It should be the same error as the rejection below. A single error means one thing. Here, you tried to reuse an error defined in another place, but you changed its meaning (from one plugin to multiple plugins and from "can't be loaded due to an error" to "can't be loaded because wasn't provided").

Reinmar · 2017-03-01T18:17:17Z

src/plugincollection.js

-		return Promise.all( plugins.map( loadPlugin ) )
+		// In order to avoid using plugin names or constructors alternatively, we map all passed plugin as plugins constructors.
+		const pluginConstructorsToLoad = plugins
+			.map( ( item ) => getPluginConstructor( item ) )


It's not an item. It's a plugin name or constructor.

Reinmar · 2017-03-01T18:34:15Z

Feature: Added support for loading plugins by name and the "removePlugins" option. Closes #49.

Kamil Piechaczek added 2 commits February 27, 2017 14:37

Feature: PluginCollection - support for loading plugins using the plu…

fa1853b

…gin names.

Feature: PluginCollection - #load() will ignored plugin listed in "re…

839ca44

…movePlugins" option.

Reinmar reviewed Feb 28, 2017

View reviewed changes

Docs: Minor improvements.

d1b12fe

Reinmar reviewed Feb 28, 2017

View reviewed changes

Kamil Piechaczek added 3 commits February 28, 2017 09:37

Revert: Editor does not support built-in plugins yet.

14cd006

Code style: Split too long line.

0d9e547

Fix: Name of plugins are optional.

49a281a

Reinmar reviewed Feb 28, 2017

View reviewed changes

Kamil Piechaczek added 2 commits February 28, 2017 11:23

Docs: Fixed invalid keys for a Map ("_availablePlugins") in PluginCol…

71b38cf

…lection.

Code style: Use operator "equals" instead of "identities" for compari…

1e7b1f6

…ng the `typeof` results.

pomek force-pushed the t/49 branch from e66d327 to 1e7b1f6 Compare February 28, 2017 10:38

Reinmar reviewed Feb 28, 2017

View reviewed changes

Kamil Piechaczek and others added 4 commits February 28, 2017 12:02

Docs: Minor updates.

a5d4686

Merge branch 'master' into t/49

c30db0d

Docs and comments fixes.

3c29e04

Improved tests.

394693e

Reinmar added the status:review- label Feb 28, 2017

Internal: Simplified the logic of PluginCollection.load() method.

e8c49ef

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

Reinmar reviewed Mar 1, 2017

View reviewed changes

Internal: Simplified the logic for extracting plugins which will not …

53068d4

…be loaded (cause they are not defined).

Internal: Removed checking whether the dependencies of loaded plugin …

80771ad

…exist.

Reinmar reviewed Mar 1, 2017

View reviewed changes

Reinmar added 2 commits March 1, 2017 19:29

Minor refactoring and docs fixes.

e0923a4

Even better API docs.

4a0e70f

Reinmar merged commit dfee52e into master Mar 1, 2017

Reinmar deleted the t/49 branch March 1, 2017 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T/49 #70

T/49 #70

pomek commented Feb 28, 2017 •

edited

Loading

Reinmar Feb 28, 2017

Reinmar Feb 28, 2017

Reinmar Feb 28, 2017

pomek Feb 28, 2017

Reinmar Feb 28, 2017

Reinmar Feb 28, 2017

Reinmar Feb 28, 2017

Reinmar Feb 28, 2017

Reinmar commented Feb 28, 2017

pomek commented Mar 1, 2017

Reinmar Mar 1, 2017

Reinmar Mar 1, 2017

Reinmar Mar 1, 2017

Reinmar Mar 1, 2017

Reinmar Mar 1, 2017

Reinmar Mar 1, 2017

Reinmar Mar 1, 2017

Reinmar Mar 1, 2017

Reinmar Mar 1, 2017

Reinmar Mar 1, 2017

Reinmar commented Mar 1, 2017

T/49 #70

T/49 #70

Conversation

pomek commented Feb 28, 2017 • edited Loading

Suggested merge commit message (convention)

Additional information

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

Reinmar commented Feb 28, 2017

pomek commented Mar 1, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar commented Mar 1, 2017

pomek commented Feb 28, 2017 •

edited

Loading