Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

T/49 #70

Merged
merged 17 commits into from
Mar 1, 2017
Merged

T/49 #70

Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/editor/editor.js
Original file line number Diff line number Diff line change
Expand Up @@ -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' ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* Commands registered to the editor.
Expand Down
95 changes: 85 additions & 10 deletions src/plugincollection.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,31 @@ export default class PluginCollection {
* Creates an instance of the PluginCollection class, initializing it with a set of plugins.
*
* @param {module:core/editor/editor~Editor} editor
* @param {Array.<module:core/plugin>} availablePlugins
*/
constructor( editor ) {
constructor( editor, availablePlugins ) {
/**
* @protected
* @member {module:core/editor/editor~Editor} module:core/plugin~PluginCollection#_editor
*/
this._editor = editor;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*/
this._availablePlugins = new Map();

/**
* @protected
* @member {Map} module:core/plugin~PluginCollection#_plugins
*/
this._plugins = new Map();

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plugin name is optional.

}
}

/**
Expand All @@ -58,48 +70,88 @@ export default class PluginCollection {
/**
* Loads a set of plugins and add them to the collection.
*
* @param {Function[]} plugins An array of {@link module:core/plugin~Plugin plugin constructors}.
* @param {Array.<module:core/plugin~Plugin|String>} plugins An array of {@link module:core/plugin~Plugin plugin constructors}
* or plugin names.
* @param {Array.<String>} removePlugins
* @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 {Array.<module:core/plugin~Plugin>} returns.loadedPlugins The array of loaded plugins.
*/
load( plugins ) {
load( plugins, removePlugins = [] ) {
const that = this;
const editor = this._editor;
const loading = new Set();
const loaded = [];

// Plugins which should be remove can be the constructors or plugin names.
removePlugins = removePlugins.reduce( ( arr, PluginConstructorOrName ) => {
arr.push( PluginConstructorOrName );

if ( typeof PluginConstructorOrName === 'string' ) {
arr.push( getPluginConstructor( PluginConstructorOrName ) );
} else {
arr.push( PluginConstructorOrName.pluginName );
}

return arr;
}, [] );

return Promise.all( plugins.map( loadPlugin ) )
.then( () => loaded );

function loadPlugin( PluginConstructor ) {
function loadPlugin( PluginConstructorOrName ) {
// Don't load the plugin if it should be removed.
if ( removePlugins.includes( PluginConstructorOrName ) ) {
return;
}

// The plugin is already loaded or being loaded - do nothing.
if ( that.get( PluginConstructor ) || loading.has( PluginConstructor ) ) {
if ( that.get( PluginConstructorOrName ) || loading.has( PluginConstructorOrName ) ) {
return;
}

return instantiatePlugin( PluginConstructor )
return instantiatePlugin( PluginConstructorOrName )
.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 } );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


throw err;
} );
}

function instantiatePlugin( PluginConstructor ) {
function instantiatePlugin( PluginConstructorOrName ) {
return new Promise( ( resolve ) => {
const PluginConstructor = getPluginConstructor( PluginConstructorOrName );

loading.add( PluginConstructor );

assertIsPlugin( PluginConstructor );

if ( PluginConstructor.requires ) {
PluginConstructor.requires.forEach( loadPlugin );
PluginConstructor.requires.forEach( ( RequiredPluginConstructorOrName ) => {
if ( removePlugins.includes( RequiredPluginConstructorOrName ) ) {
/**
* The plugin dependency cannot be loaded because is listed in `removePlugins` options.
*
* @error plugincollection-instance
* @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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

throw new CKEditorError(
'plugincollection-instance: Cannot load dependency plugins because at least one is listed in `removePlugins` options.',
{ plugin: RequiredPluginConstructorOrName, requiredBy: PluginConstructorOrName }
);
// jscs:enable maximumLineLength
}

loadPlugin( RequiredPluginConstructorOrName );
} );
}

const plugin = new PluginConstructor( editor );
Expand All @@ -110,6 +162,29 @@ export default class PluginCollection {
} );
}

function getPluginConstructor( PluginConstructorOrName ) {
if ( typeof PluginConstructorOrName === 'function' ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use == after typeof.

return PluginConstructorOrName;
}

const PluginConstructor = that._availablePlugins.get( PluginConstructorOrName );

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*
* @error plugincollection-instance
* @param {*} plugin The constructor which is meant to be loaded as a plugin.
*/
throw new CKEditorError(
'plugincollection-instance: Given plugin name is not available.',
{ plugin: PluginConstructorOrName }
);
}

return PluginConstructor;
}

function assertIsPlugin( PluginConstructor ) {
if ( !( PluginConstructor.prototype instanceof Plugin ) ) {
/**
Expand Down
Loading