8
0
Эх сурвалжийг харах

First stage of revisiting the plugins infrastructure.

Piotrek Koszuliński 10 жил өмнө
parent
commit
0d74f8f652

+ 12 - 3
packages/ckeditor5-engine/src/creator.js

@@ -9,10 +9,19 @@
  * Basic creator class.
  *
  * @class Creator
- * @extends Plugin
  */
 
-import Plugin from './plugin.js';
+export default class Creator {
+	constructor( editor ) {
+		/**
+		 * @readonly
+		 * @property {core/Editor}
+		 */
+		this.editor = editor;
+	}
 
-export default class Creator extends Plugin {
+	/**
+	 * @returns {null/Promise}
+	 */
+	init() {}
 }

+ 3 - 10
packages/ckeditor5-engine/src/editor.js

@@ -104,16 +104,9 @@ export default class Editor extends Model {
 		}
 
 		function initPlugins( loadedPlugins ) {
-			// Start with a resolved promise.
-			let promise = Promise.resolve();
-
-			// Chain it with promises that resolve with the init() call of every plugin.
-			for ( let i = 0; i < loadedPlugins.length; i++ ) {
-				promise = promise.then( () => loadedPlugins[ i ].init() );
-			}
-
-			// Return the promise chain.
-			return promise;
+			return loadedPlugins.reduce( ( promise, plugin ) => {
+				return promise.then( plugin.init.bind( plugin ) );
+			}, Promise.resolve() );
 		}
 
 		function findCreators() {

+ 8 - 6
packages/ckeditor5-engine/src/plugin.js

@@ -9,18 +9,20 @@
  * The base class for CKEditor plugin classes.
  *
  * @class Plugin
- * @extends Model
  */
 
-import Model from './model.js';
-
-export default class Plugin extends Model {
+export default class Plugin {
 	constructor( editor ) {
-		super();
-
+		/**
+		 * @readonly
+		 * @property {core/Editor}
+		 */
 		this.editor = editor;
 	}
 
+	/**
+	 * @returns {null/Promise}
+	 */
 	init() {}
 }
 

+ 69 - 59
packages/ckeditor5-engine/src/plugincollection.js

@@ -9,24 +9,29 @@
  * Manages a list of CKEditor plugins, including loading, initialization and destruction.
  *
  * @class PluginCollection
- * @extends Collection
+ * @extends Map
  */
 
-import Collection from './collection.js';
+import CKEDITOR from '../ckeditor.js';
 import Plugin from './plugin.js';
 import CKEditorError from './ckeditorerror.js';
 import log from './log.js';
 import load from '../ckeditor5/load.js';
 
-export default class PluginCollection extends Collection {
+export default class PluginCollection extends Map {
 	/**
 	 * Creates an instance of the PluginCollection class, initializing it with a set of plugins.
 	 *
 	 * @constructor
+	 * @param {core/Editor} editor
 	 */
 	constructor( editor ) {
-		super( { idProperty: 'name' } );
+		super();
 
+		/**
+		 * @protected
+		 * @property {core/Editor}
+		 */
 		this._editor = editor;
 	}
 
@@ -39,68 +44,28 @@ export default class PluginCollection extends Collection {
 	 * @param {core/Plugin[]} returns.loadedPlugins The array of loaded plugins.
 	 */
 	load( plugins ) {
-		// The list of plugins which are being loaded (to avoid circular references issues).
-		const loading = {};
-		// Plugins added to the collection (for the purpose of returning an array of loaded plugins).
-		const loaded = [];
 		const that = this;
+		const editor = this._editor;
+		const loading = new Set();
+		const loaded = [];
 
 		// It may happen that an empty list was passed – don't fail.
 		plugins = plugins ? plugins.split( ',' ) : [];
 
-		// Creates a promise for the loading of each plugin and returns a main promise that resolves when all are
-		// done.
-		return Promise.all( plugins.map( pluginPromise ) )
+		return Promise.all( plugins.map( loadPlugin ) )
 			.then( () => loaded );
 
-		// Returns a promise that will load the plugin and add it to the collection before resolving.
-		function pluginPromise( plugin ) {
-			// Do nothing if the plugin is already loaded (or if is being loaded right now).
-			if ( that.get( plugin ) || loading[ plugin ] ) {
-				return Promise.resolve();
+		function loadPlugin( pluginClassOrName ) {
+			// The plugin is already loaded or being loaded - do nothing.
+			if ( that.get( pluginClassOrName ) || loading.has( pluginClassOrName ) ) {
+				return;
 			}
 
-			return load( `../ckeditor5-${ plugin }/${ plugin }.js` )
-				.then( ( LoadedPluginModule ) => {
-					const LoadedPlugin = LoadedPluginModule.default;
-					const deps = LoadedPluginModule.requires || [];
-
-					const isPluginDep = plugin.indexOf( '/' ) > 0;
-					let loadedPlugin;
-
-					if ( !isPluginDep ) {
-						loadedPlugin = new LoadedPlugin( that._editor );
-
-						if ( !( loadedPlugin instanceof Plugin ) ) {
-							/**
-							 * The plugin is not an instance of Plugin.
-							 *
-							 * @error plugincollection-instance
-							 * @param {String} plugin The name of the plugin that is not an instance of Plugin.
-							 */
-							throw new CKEditorError(
-								'plugincollection-instance: The plugin is not an instance of Plugin.',
-								{ plugin: plugin }
-							);
-						}
-
-						loadedPlugin.name = plugin;
-						loadedPlugin.deps = deps;
-					}
-
-					loading[ plugin ] = true;
-
-					// Resolve with a promise that resolves once all dependencies are loaded.
-					return Promise.all( deps.map( pluginPromise ) )
-						.then( () => {
-							// Once dependencies are loaded, add the new instance of the loaded plugin to
-							// the collection. This guarantees that dependecies come first in the collection.
-							if ( !isPluginDep ) {
-								that.add( loadedPlugin );
-								loaded.push( loadedPlugin );
-							}
-						} );
-				} )
+			let promise = ( typeof pluginClassOrName == 'string' ) ?
+				loadPluginByName( pluginClassOrName ) :
+				loadPluginByClass( pluginClassOrName );
+
+			return promise
 				.catch( ( err ) => {
 					/**
 					 * It was not possible to load the plugin.
@@ -108,10 +73,55 @@ export default class PluginCollection extends Collection {
 					 * @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: plugin } );
+					log.error( 'plugincollection-load: It was not possible to load the plugin.', { plugin: pluginClassOrName } );
+
+					throw err;
+				} );
+		}
 
-					return err;
+		function loadPluginByName( pluginName ) {
+			return load( CKEDITOR.getModulePath( pluginName ) )
+				.then( ( PluginModule ) => {
+					return loadPluginByClass( PluginModule.default, pluginName );
 				} );
 		}
+
+		function loadPluginByClass( PluginClass, pluginName ) {
+			return new Promise( ( resolve ) => {
+				loading.add( PluginClass );
+
+				assertIsPlugin( PluginClass );
+
+				if ( PluginClass.requires ) {
+					PluginClass.requires.forEach( loadPlugin );
+				}
+
+				const plugin = new PluginClass( editor );
+				that.set( PluginClass, plugin );
+				loaded.push( plugin );
+
+				// Expose the plugin also by its name if loaded through load() by name.
+				if ( pluginName ) {
+					that.set( pluginName, plugin );
+				}
+
+				resolve();
+			} );
+		}
+
+		function assertIsPlugin( LoadedPlugin ) {
+			if ( !( LoadedPlugin.prototype instanceof Plugin ) ) {
+				/**
+				 * The plugin is not an instance of Plugin.
+				 *
+				 * @error plugincollection-instance
+				 * @param {LoadedPlugin} plugin The class which is meant to be loaded as a plugin.
+				 */
+				throw new CKEditorError(
+					'plugincollection-instance: The plugin is not an instance of Plugin.',
+					{ plugin: LoadedPlugin }
+				);
+			}
+		}
 	}
 }

+ 106 - 119
packages/ckeditor5-engine/tests/plugincollection.js

@@ -5,11 +5,18 @@
 
 'use strict';
 
-const modules = bender.amd.require( 'core/plugincollection', 'core/plugin', 'core/editor', 'core/log' );
-let PluginCollection, Plugin, Editor, log;
+const modules = bender.amd.require(
+	'core/editor',
+	'core/plugincollection',
+	'core/plugin',
+	'core/creator',
+	'core/ckeditorerror',
+	'core/log'
+);
+let PluginCollection, Plugin, Editor, Creator, CKEditorError, log;
 
 let editor;
-let PluginA, PluginB;
+let PluginA, PluginB, PluginC, PluginD, PluginE, PluginF;
 class TestError extends Error {}
 
 bender.tools.createSinonSandbox();
@@ -18,10 +25,21 @@ before( () => {
 	PluginCollection = modules[ 'core/plugincollection' ];
 	Editor = modules[ 'core/editor' ];
 	Plugin = modules[ 'core/plugin' ];
+	Creator = modules[ 'core/creator' ];
+	CKEditorError = modules[ 'core/ckeditorerror' ];
 	log = modules[ 'core/log' ];
 
-	PluginA = class extends Plugin {};
-	PluginB = class extends Plugin {};
+	PluginA = createPlugin( 'A' );
+	PluginB = createPlugin( 'B' );
+	PluginC = createPlugin( 'C' );
+	PluginD = createPlugin( 'D' );
+	PluginE = createPlugin( 'E' );
+	PluginF = createPlugin( 'F' );
+
+	PluginC.requires = [ PluginB ];
+	PluginD.requires = [ PluginA, PluginC ];
+	PluginF.requires = [ PluginE ];
+	PluginE.requires = [ PluginF ];
 
 	editor = new Editor( document.body.appendChild( document.createElement( 'div' ) ) );
 } );
@@ -36,55 +54,30 @@ bender.amd.define( 'B', () => {
 	return PluginB;
 } );
 
-bender.amd.define( 'C', [ 'core/plugin', 'B' ], ( Plugin ) => {
-	return class extends Plugin {};
+bender.amd.define( 'C', [ 'core/plugin', 'B' ], () => {
+	return PluginC;
 } );
 
-bender.amd.define( 'D', [ 'core/plugin', 'A', 'C' ], ( Plugin ) => {
-	return class extends Plugin {};
+bender.amd.define( 'D', [ 'core/plugin', 'A', 'C' ], () => {
+	return PluginD;
 } );
 
-bender.amd.define( 'E', [ 'core/plugin', 'F' ], ( Plugin ) => {
-	return class extends Plugin {};
+bender.amd.define( 'E', [ 'core/plugin', 'F' ], () => {
+	return PluginE;
 } );
 
-bender.amd.define( 'F', [ 'core/plugin', 'E' ], ( Plugin ) => {
-	return class extends Plugin {};
+bender.amd.define( 'F', [ 'core/plugin', 'E' ], () => {
+	return PluginF;
 } );
 
+// Erroneous cases.
+
 bender.amd.define( 'G', () => {
 	throw new TestError( 'Some error inside a plugin' );
 } );
 
-bender.amd.define( 'H', [ 'core/plugin', 'H/a' ], ( Plugin ) => {
-	return class extends Plugin {};
-} );
-
-let spies = {};
-// Note: This is NOT a plugin.
-bender.amd.define( 'H/a', [ 'H/a/b' ], () => {
-	return ( spies[ 'H/a' ] = sinon.spy() );
-} );
-
-// Note: This is NOT a plugin.
-bender.amd.define( 'H/a/b', [ 'c' ], () => {
-	return ( spies[ 'H/a/b' ] = sinon.spy() );
-} );
-
-// Note: This is NOT a plugin.
-bender.amd.define( 'c', () => {
-	return ( spies.c = sinon.spy() );
-} );
-
-bender.amd.define( 'I', [ 'core/plugin', 'J' ], ( Plugin ) => {
-	return class extends Plugin {};
-} );
-
-// Note: This is NOT a plugin.
-bender.amd.define( 'J', () => {
-	return function() {
-		return ( spies.jSpy = sinon.spy() );
-	};
+bender.amd.define( 'I', () => {
+	return class {};
 } );
 
 /////////////
@@ -95,7 +88,7 @@ describe( 'load', () => {
 
 		return plugins.load( '' )
 			.then( () => {
-				expect( plugins.length ).to.equal( 0 );
+				expect( getPlugins( plugins ) ).to.be.empty();
 			} );
 	} );
 
@@ -104,7 +97,7 @@ describe( 'load', () => {
 
 		return plugins.load()
 			.then( () => {
-				expect( plugins.length ).to.equal( 0 );
+				expect( getPlugins( plugins ) ).to.be.empty();
 			} );
 	} );
 
@@ -113,7 +106,10 @@ describe( 'load', () => {
 
 		return plugins.load( 'A,B' )
 			.then( () => {
-				expect( plugins.length ).to.equal( 2 );
+				expect( getPlugins( plugins ).length ).to.equal( 2 );
+
+				expect( plugins.get( PluginA ) ).to.be.an.instanceof( PluginA );
+				expect( plugins.get( PluginB ) ).to.be.an.instanceof( PluginB );
 
 				expect( plugins.get( 'A' ) ).to.be.an.instanceof( PluginA );
 				expect( plugins.get( 'B' ) ).to.be.an.instanceof( PluginB );
@@ -122,55 +118,63 @@ describe( 'load', () => {
 
 	it( 'should load dependency plugins', () => {
 		let plugins = new PluginCollection( editor );
-		let spy = sinon.spy( plugins, 'add' );
+		let spy = sinon.spy( plugins, 'set' );
 
 		return plugins.load( 'A,C' )
 			.then( ( loadedPlugins ) => {
-				expect( plugins.length ).to.equal( 3 );
+				expect( getPlugins( plugins ).length ).to.equal( 3 );
 
-				expect( getPluginNamesFromSpy( spy ) ).to.deep.equal( [ 'A', 'B', 'C' ], 'order by plugins.add()' );
-				expect( getPluginNames( loadedPlugins ) ).to.deep.equal( [ 'A', 'B', 'C' ], 'order by returned value' );
+				expect( getPluginNames( getPluginsFromSpy( spy ) ) )
+					.to.deep.equal( [ 'A', 'B', 'C' ], 'order by plugins.set()' );
+				expect( getPluginNames( loadedPlugins ) )
+					.to.deep.equal( [ 'A', 'B', 'C' ], 'order by returned value' );
 			} );
 	} );
 
 	it( 'should be ok when dependencies are loaded first', () => {
 		let plugins = new PluginCollection( editor );
-		let spy = sinon.spy( plugins, 'add' );
+		let spy = sinon.spy( plugins, 'set' );
 
 		return plugins.load( 'A,B,C' )
 			.then( ( loadedPlugins ) => {
-				expect( plugins.length ).to.equal( 3 );
+				expect( getPlugins( plugins ).length ).to.equal( 3 );
 
-				expect( getPluginNamesFromSpy( spy ) ).to.deep.equal( [ 'A', 'B', 'C' ], 'order by plugins.add()' );
-				expect( getPluginNames( loadedPlugins ) ).to.deep.equal( [ 'A', 'B', 'C' ], 'order by returned value' );
+				expect( getPluginNames( getPluginsFromSpy( spy ) ) )
+					.to.deep.equal( [ 'A', 'B', 'C' ], 'order by plugins.set()' );
+				expect( getPluginNames( loadedPlugins ) )
+					.to.deep.equal( [ 'A', 'B', 'C' ], 'order by returned value' );
 			} );
 	} );
 
 	it( 'should load deep dependency plugins', () => {
 		let plugins = new PluginCollection( editor );
-		let spy = sinon.spy( plugins, 'add' );
+		let spy = sinon.spy( plugins, 'set' );
 
 		return plugins.load( 'D' )
 			.then( ( loadedPlugins ) => {
-				expect( plugins.length ).to.equal( 4 );
+				expect( getPlugins( plugins ).length ).to.equal( 4 );
 
 				// The order must have dependencies first.
-				expect( getPluginNamesFromSpy( spy ) ).to.deep.equal( [ 'A', 'B', 'C', 'D' ], 'order by plugins.add()' );
-				expect( getPluginNames( loadedPlugins ) ).to.deep.equal( [ 'A', 'B', 'C', 'D' ], 'order by returned value' );
+				expect( getPluginNames( getPluginsFromSpy( spy ) ) )
+					.to.deep.equal( [ 'A', 'B', 'C', 'D' ], 'order by plugins.set()' );
+				expect( getPluginNames( loadedPlugins ) )
+					.to.deep.equal( [ 'A', 'B', 'C', 'D' ], 'order by returned value' );
 			} );
 	} );
 
 	it( 'should handle cross dependency plugins', () => {
 		let plugins = new PluginCollection( editor );
-		let spy = sinon.spy( plugins, 'add' );
+		let spy = sinon.spy( plugins, 'set' );
 
 		return plugins.load( 'A,E' )
 			.then( ( loadedPlugins ) => {
-				expect( plugins.length ).to.equal( 3 );
+				expect( getPlugins( plugins ).length ).to.equal( 3 );
 
 				// The order must have dependencies first.
-				expect( getPluginNamesFromSpy( spy ) ).to.deep.equal( [ 'A', 'F', 'E' ], 'order by plugins.add()' );
-				expect( getPluginNames( loadedPlugins ) ).to.deep.equal( [ 'A', 'F', 'E' ], 'order by returned value' );
+				expect( getPluginNames( getPluginsFromSpy( spy ) ) )
+					.to.deep.equal( [ 'A', 'F', 'E' ], 'order by plugins.set()' );
+				expect( getPluginNames( loadedPlugins ) )
+					.to.deep.equal( [ 'A', 'F', 'E' ], 'order by returned value' );
 			} );
 	} );
 
@@ -184,18 +188,6 @@ describe( 'load', () => {
 			} );
 	} );
 
-	it( 'should set the `deps` property on loaded plugins', () => {
-		let plugins = new PluginCollection( editor );
-
-		return plugins.load( 'A,D' )
-			.then( () => {
-				expect( plugins.get( 'A' ).deps ).to.deep.equal( [] );
-				expect( plugins.get( 'B' ).deps ).to.deep.equal( [] );
-				expect( plugins.get( 'C' ).deps ).to.deep.equal( [ 'B' ] );
-				expect( plugins.get( 'D' ).deps ).to.deep.equal( [ 'A', 'C' ] );
-			} );
-	} );
-
 	it( 'should reject on invalid plugin names (forward require.js loading error)', () => {
 		let logSpy = bender.sinon.stub( log, 'error' );
 
@@ -235,64 +227,59 @@ describe( 'load', () => {
 			} );
 	} );
 
-	it( 'should load `deps` which are not plugins', () => {
-		let plugins = new PluginCollection( editor );
-		expect( spies ).to.be.empty;
-
-		return plugins.load( 'H' )
-			.then( () => {
-				expect( plugins.get( 'H' ).deps ).to.deep.equal( [ 'H/a' ] );
-
-				// Non–plugin dependencies should be loaded (spy exists)...
-				expect( spies ).to.have.keys( [
-					'H/a', 'H/a/b', 'c'
-				] );
-
-				// ...but not be executed (called == false)...
-				expect( spies[ 'H/a' ].called ).to.be.false;
-				expect( spies[ 'H/a/b' ].called ).to.be.false;
-				expect( spies.c.called ).to.be.false;
-
-				expect( plugins.length ).to.be.equal( 1 );
-			} );
-	} );
+	it( 'should reject when loading a module which is not a plugin', () => {
+		let logSpy = bender.sinon.stub( log, 'error' );
 
-	it( 'should load instances of Plugin only', () => {
 		let plugins = new PluginCollection( editor );
 
 		return plugins.load( 'I' )
+			// Throw here, so if by any chance plugins.load() was resolved correctly catch() will be stil executed.
 			.then( () => {
 				throw new Error( 'Test error: this promise should not be resolved successfully' );
-			} ).catch( err => {
-				expect( err.name ).to.be.equal( 'CKEditorError' );
-				expect( err.message ).to.match( /^plugincollection-instance:/ );
-			} );
-	} );
-
-	it( 'should cancel loading module which looks like a plugin but is a normal module', () => {
-		let plugins = new PluginCollection( editor );
+			} )
+			.catch( ( err ) => {
+				expect( err ).to.be.an.instanceof( CKEditorError );
+				expect( err.message ).to.match( /^plugincollection-instance/ );
 
-		return plugins.load( 'J' )
-			.then( () => {
-				throw new Error( 'Test error: this promise should not be resolved successfully' );
-			} ).catch( () => {
-				// Path would be set if code execution wasn't stopped when we rejected the promise
-				// (based on a real mistake we've made).
-				// expect( spies.jSpy.path ).to.be.undefined;
-				//
-				// TODO now, the above does not make sense, because we removed the path.
+				sinon.assert.calledOnce( logSpy );
+				expect( logSpy.args[ 0 ][ 0 ] ).to.match( /^plugincollection-load:/ );
 			} );
 	} );
 } );
 
-function getPluginNamesFromSpy( addSpy ) {
-	return addSpy.args.map( ( arg ) => {
-		return arg[ 0 ].name;
-	} );
+function createPlugin( name ) {
+	const P = class extends Plugin {
+		constructor( editor ) {
+			super( editor );
+			this._pluginName = name;
+		}
+	};
+
+	P._pluginName = name;
+
+	return P;
+}
+
+function getPlugins( pluginCollection ) {
+	const plugins = [];
+
+	for ( let entry of pluginCollection.entries() ) {
+		if ( typeof entry[ 0 ] == 'function' ) {
+			plugins.push( entry[ 1 ] );
+		}
+	}
+
+	return plugins;
+}
+
+function getPluginsFromSpy( addSpy ) {
+	return addSpy.args
+		.map( ( arg ) => arg[ 0 ] )
+		// Entries may be kept twice in the plugins map - once as a pluginName => plugin, once as pluginClass => plugin.
+		// Return only pluginClass => plugin entries as these will always represent all plugins.
+		.filter( ( plugin ) => typeof plugin == 'function' );
 }
 
 function getPluginNames( plugins ) {
-	return plugins.map( ( arg ) => {
-		return arg.name;
-	} );
+	return plugins.map( ( plugin ) => plugin._pluginName );
 }