Przeglądaj źródła

Merge pull request #43 from cksource/t/42

t/42: Plugincollection is greedy while loading plugin dependencies
Piotrek Koszuliński 10 lat temu
rodzic
commit
046fb4b974

+ 31 - 7
packages/ckeditor5-engine/src/plugincollection.js

@@ -12,7 +12,13 @@
  * @extends Collection
  */
 
-CKEDITOR.define( [ 'collection', 'promise', 'log' ], function( Collection, Promise, log ) {
+CKEDITOR.define( [
+	'collection',
+	'promise',
+	'plugin',
+	'ckeditorerror',
+	'log'
+], function( Collection, Promise, Plugin, CKEditorError, log ) {
 	class PluginCollection extends Collection {
 		/**
 		 * Creates an instance of the PluginCollection class, initializing it with a set of plugins.
@@ -58,20 +64,38 @@ CKEDITOR.define( [ 'collection', 'promise', 'log' ], function( Collection, Promi
 					CKEDITOR.require( [ 'plugin!' + plugin ],
 						// Success callback.
 						function( LoadedPlugin ) {
-							var loadedPlugin = new LoadedPlugin( that._editor );
-							loadedPlugin.name = plugin;
-							loadedPlugin.path = CKEDITOR.getPluginPath( plugin );
-							loadedPlugin.deps = getPluginDeps( plugin );
+							var deps = getPluginDeps( plugin );
+							var isPluginDep = plugin.indexOf( '/' ) > 0;
+
+							if ( !isPluginDep ) {
+								var 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.
+									 */
+									reject( new CKEditorError( 'plugincollection-instance: The plugin is not an instance of Plugin.', { plugin: plugin } ) );
+								}
+
+								loadedPlugin.name = plugin;
+								loadedPlugin.path = CKEDITOR.getPluginPath( plugin );
+								loadedPlugin.deps = deps;
+							}
 
 							loading[ plugin ] = true;
 
 							// Resolve with a promise that resolves once all dependencies are loaded.
 							resolve(
-								Promise.all( loadedPlugin.deps.map( pluginPromise ) )
+								Promise.all( deps.map( pluginPromise ) )
 									.then( function() {
 										// Once dependencies are loaded, add the new instance of the loaded plugin to
 										// the collection. This guarantees that dependecies come first in the collection.
-										that.add( loadedPlugin );
+										if ( !isPluginDep ) {
+											that.add( loadedPlugin );
+										}
 									} )
 							);
 						},

+ 66 - 0
packages/ckeditor5-engine/tests/plugincollection/plugincollection.js

@@ -61,6 +61,35 @@ CKEDITOR.define( 'plugin!G', function() {
 	throw new TestError( 'Some error inside a plugin' );
 } );
 
+CKEDITOR.define( 'plugin!H', [ 'plugin', 'plugin!H/a' ], function( Plugin ) {
+	return class extends Plugin {};
+} );
+
+var spies = {};
+// Note: This is NOT a plugin.
+CKEDITOR.define( 'plugin!H/a', [ 'plugin!H/a/b' ], function() {
+	return ( spies[ 'plugin!H/a' ] = sinon.spy() );
+} );
+
+// Note: This is NOT a plugin.
+CKEDITOR.define( 'plugin!H/a/b', [ 'c' ], function() {
+	return ( spies[ 'plugin!H/a/b' ] = sinon.spy() );
+} );
+
+// Note: This is NOT a plugin.
+CKEDITOR.define( 'c', function() {
+	return ( spies.c = sinon.spy() );
+} );
+
+CKEDITOR.define( 'plugin!I', [ 'plugin', 'plugin!J' ], function( Plugin ) {
+	return class extends Plugin {};
+} );
+
+// Note: This is NOT a plugin.
+CKEDITOR.define( 'plugin!J', function() {
+	return class {};
+} );
+
 /////////////
 
 describe( 'load', function() {
@@ -225,6 +254,43 @@ describe( 'load', function() {
 				expect( logSpy.args[ 0 ][ 0 ] ).to.match( /^plugincollection-load:/ );
 			} );
 	} );
+
+	it( 'should load `deps` which are not plugins', function() {
+		var PluginCollection = modules.plugincollection;
+
+		var plugins = new PluginCollection( editor );
+		expect( spies ).to.be.empty;
+
+		return plugins.load( 'H' )
+			.then( function() {
+				expect( plugins.get( 'H' ).deps ).to.deep.equal( [ 'H/a' ] );
+
+				// Non–plugin dependencies should be loaded (spy exists)...
+				expect( spies ).to.have.keys( [
+					'plugin!H/a', 'plugin!H/a/b', 'c'
+				] );
+
+				// ...but not be executed (called == false)...
+				expect( spies[ 'plugin!H/a' ].called ).to.be.false;
+				expect( spies[ 'plugin!H/a/b' ].called ).to.be.false;
+				expect( spies.c.called ).to.be.false;
+
+				expect( plugins.length ).to.be.equal( 1 );
+			} );
+	} );
+
+	it( 'should load instances of Plugin only', function() {
+		var PluginCollection = modules.plugincollection;
+		var plugins = new PluginCollection( editor );
+
+		return plugins.load( 'I' )
+			.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:/ );
+			} );
+	} );
 } );
 
 describe( 'add', function() {