diff --git a/src/plugin.js b/src/plugin.js index 323b7f5a..3e55420e 100644 --- a/src/plugin.js +++ b/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 diff --git a/src/plugincollection.js b/src/plugincollection.js index df3a5567..96ff66c6 100644 --- a/src/plugincollection.js +++ b/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 ); } } } diff --git a/tests/plugincollection.js b/tests/plugincollection.js index 9064d40f..2d28ba4f 100644 --- a/tests/plugincollection.js +++ b/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()', () => {