Merge pull request #95 from ckeditor/t/85

Feature: `PluginCollection` will warning if the user wants to load two or more plugins with the same name. Closes #85.

NOTE: Plugin naming convention has changed.

src/plugin.js

@@ -108,18 +108,19 @@ mix( Plugin, ObservableMixin );
  * {@link module:core/plugincollection~PluginCollection#get} by its
  * name and its constructor. If not, then only by its constructor.
  *
- * The name should reflect the package name + the plugin module name. E.g. `ckeditor5-image/src/image.js` plugin
- * should be named `image/image`. If plugin is kept deeper in the directory structure, it's recommended to only use the module file name,
- * not the whole path. So, e.g. a plugin defined in `ckeditor5-ui/src/notification/notification.js` file may be named `ui/notification`.
+ * The name should reflect the constructor name.
  *
  * To keep a plugin class definition tight it's recommended to define this property as a static getter:
  *
  *		export default class ImageCaption {
  *			static get pluginName() {
- *				return 'image/imagecaption';
+ *				return 'ImageCaption';
  *			}
  *		}
  *
+ * Note: The native `Function.name` property could not be used to keep the plugin name because
+ * it will be mangled during code minification.
+ *
  * @static
  * @readonly
  * @member {String|undefined} module:core/plugin~PluginInterface.pluginName

src/plugincollection.js

@@ -231,8 +231,29 @@ export default class PluginCollection {
 	_add( PluginConstructor, plugin ) {
 		this._plugins.set( PluginConstructor, plugin );
 
-		if ( PluginConstructor.pluginName ) {
-			this._plugins.set( PluginConstructor.pluginName, plugin );
+		const pluginName = PluginConstructor.pluginName;
+
+		if ( !pluginName ) {
+			return;
+		}
+
+		if ( this._plugins.has( pluginName ) ) {
+			/**
+			 * Two plugins with the same {@link module:core/plugin~PluginInterface.pluginName} were loaded.
+			 * This may lead to runtime conflicts between these plugins. This usually means that incorrect
+			 * params were passed to {@link module:core/editor/editor~Editor.create}.
+			 *
+			 * @error plugincollection-plugin-name-conflict
+			 * @param {String} pluginName The duplicated plugin name.
+			 * @param {Function} plugin1 The first plugin constructor.
+			 * @param {Function} plugin2 The second plugin constructor.
+			 */
+			log.warn(
+				'plugincollection-plugin-name-conflict: Two plugins with the same name were loaded.',
+				{ pluginName, plugin1: this._plugins.get( pluginName ).constructor, plugin2: PluginConstructor }
+			);
+		} else {
+			this._plugins.set( pluginName, plugin );
 		}
 	}
 }

tests/plugincollection.js

@@ -13,7 +13,7 @@ import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 import log from '@ckeditor/ckeditor5-utils/src/log';
 
 let editor, availablePlugins;
-let PluginA, PluginB, PluginC, PluginD, PluginE, PluginF, PluginG, PluginH, PluginI, PluginJ, PluginK, PluginX;
+let PluginA, PluginB, PluginC, PluginD, PluginE, PluginF, PluginG, PluginH, PluginI, PluginJ, PluginK, PluginX, PluginFoo, AnotherPluginFoo;
 class TestError extends Error {}
 class ChildPlugin extends Plugin {}
 class GrandPlugin extends ChildPlugin {}
@@ -39,6 +39,8 @@ before( () => {
 			throw new TestError( 'Some error inside a plugin' );
 		}
 	};
+	PluginFoo = createPlugin( 'Foo' );
+	AnotherPluginFoo = createPlugin( 'Foo' );
 
 	PluginC.requires = [ PluginB ];
 	PluginD.requires = [ PluginA, PluginC ];
@@ -67,6 +69,8 @@ describe( 'PluginCollection', () => {
 			PluginK,
 			PluginX
 		];
+
+		PluginFoo.requires = [];
 	} );
 
 	describe( 'load()', () => {
@@ -351,6 +355,69 @@ describe( 'PluginCollection', () => {
 					expect( logSpy.calledTwice ).to.equal( true );
 				} );
 		} );
+
+		it( 'logs if tries to load more than one plugin with the same name', () => {
+			const logSpy = testUtils.sinon.stub( log, 'warn' );
+			const plugins = new PluginCollection( editor );
+
+			return plugins.load( [ PluginFoo, AnotherPluginFoo ] )
+				.then( () => {
+					expect( getPlugins( plugins ).length ).to.equal( 2 );
+
+					expect( plugins.get( 'Foo' ) ).to.be.an.instanceof( PluginFoo );
+					expect( plugins.get( PluginFoo ) ).to.be.an.instanceof( PluginFoo );
+					expect( plugins.get( AnotherPluginFoo ) ).to.be.an.instanceof( AnotherPluginFoo );
+
+					expect( logSpy.calledOnce ).to.equal( true );
+					expect( logSpy.firstCall.args[ 0 ] ).to.match( /^plugincollection-plugin-name-conflict:/ );
+
+					const warnData = logSpy.firstCall.args[ 1 ];
+					expect( warnData.pluginName ).to.equal( 'Foo' );
+					expect( warnData.plugin1 ).to.equal( PluginFoo );
+					expect( warnData.plugin2 ).to.equal( AnotherPluginFoo );
+				} );
+		} );
+
+		it( 'logs if tries to load more than one plugin with the same name (plugin requires plugin with the same name)', () => {
+			PluginFoo.requires = [ AnotherPluginFoo ];
+
+			const logSpy = testUtils.sinon.stub( log, 'warn' );
+			const plugins = new PluginCollection( editor );
+
+			return plugins.load( [ PluginFoo ] )
+				.then( () => {
+					expect( getPlugins( plugins ).length ).to.equal( 2 );
+
+					expect( plugins.get( 'Foo' ) ).to.be.an.instanceof( AnotherPluginFoo );
+					expect( plugins.get( AnotherPluginFoo ) ).to.be.an.instanceof( AnotherPluginFoo );
+					expect( plugins.get( PluginFoo ) ).to.be.an.instanceof( PluginFoo );
+
+					expect( logSpy.calledOnce ).to.equal( true );
+					expect( logSpy.firstCall.args[ 0 ] ).to.match( /^plugincollection-plugin-name-conflict:/ );
+				} );
+		} );
+
+		it(
+			'logs if tries to load more than one plugin with the same name (plugin with the same name is built-in the PluginCollection)',
+			() => {
+				availablePlugins = [ PluginFoo ];
+
+				const logSpy = testUtils.sinon.stub( log, 'warn' );
+				const plugins = new PluginCollection( editor, availablePlugins );
+
+				return plugins.load( [ 'Foo', AnotherPluginFoo ] )
+					.then( () => {
+						expect( getPlugins( plugins ).length ).to.equal( 2 );
+
+						expect( plugins.get( 'Foo' ) ).to.be.an.instanceof( PluginFoo );
+						expect( plugins.get( PluginFoo ) ).to.be.an.instanceof( PluginFoo );
+						expect( plugins.get( AnotherPluginFoo ) ).to.be.an.instanceof( AnotherPluginFoo );
+
+						expect( logSpy.calledOnce ).to.equal( true );
+						expect( logSpy.firstCall.args[ 0 ] ).to.match( /^plugincollection-plugin-name-conflict:/ );
+					} );
+			}
+		);
 	} );
 
 	describe( 'get()', () => {