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 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
fa1853b
Feature: PluginCollection - support for loading plugins using the plu…
Feb 27, 2017
839ca44
Feature: PluginCollection - #load() will ignored plugin listed in "re…
Feb 28, 2017
d1b12fe
Docs: Minor improvements.
Feb 28, 2017
14cd006
Revert: Editor does not support built-in plugins yet.
Feb 28, 2017
0d9e547
Code style: Split too long line.
Feb 28, 2017
49a281a
Fix: Name of plugins are optional.
Feb 28, 2017
71b38cf
Docs: Fixed invalid keys for a Map ("_availablePlugins") in PluginCol…
Feb 28, 2017
1e7b1f6
Code style: Use operator "equals" instead of "identities" for compari…
Feb 28, 2017
a5d4686
Docs: Minor updates.
Feb 28, 2017
c30db0d
Merge branch 'master' into t/49
Reinmar Feb 28, 2017
3c29e04
Docs and comments fixes.
Reinmar Feb 28, 2017
394693e
Improved tests.
Reinmar Feb 28, 2017
e8c49ef
Internal: Simplified the logic of `PluginCollection.load()` method.
Mar 1, 2017
53068d4
Internal: Simplified the logic for extracting plugins which will not …
Mar 1, 2017
80771ad
Internal: Removed checking whether the dependencies of loaded plugin …
Mar 1, 2017
e0923a4
Minor refactoring and docs fixes.
Reinmar Mar 1, 2017
4a0e70f
Even better API docs.
Reinmar Mar 1, 2017
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
111 changes: 61 additions & 50 deletions src/plugincollection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,83 +86,114 @@ export default class PluginCollection {
* which should not be loaded (despite being specified in the `plugins` array).
* @returns {Promise} A promise which gets resolved once all plugins are loaded and available into the
* collection.
* @returns {Array.<module:core/plugin~Plugin>} returns.loadedPlugins The array of loaded plugins.
* @returns {Promise.<Array.<module:core/plugin~Plugin>>} returns.loadedPlugins The array of loaded 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 optional.

*/
load( plugins, removePlugins = [] ) {
const that = this;
const editor = this._editor;
const loading = new Set();
const loaded = [];

// Plugins that will be removed can be the constructors or names.
// We need to unify this because we are supporting loading plugins using both types.
removePlugins = removePlugins.reduce( ( arr, PluginConstructorOrName ) => {
arr.push( PluginConstructorOrName );

if ( typeof PluginConstructorOrName == 'string' ) {
arr.push( getPluginConstructor( PluginConstructorOrName ) );
} else if ( PluginConstructorOrName.pluginName ) {
arr.push( PluginConstructorOrName.pluginName );
}
// In order to avoid using plugin names or constructors alternatively, we map all passed plugin as plugins constructors.
const pluginConstructorsToLoad = plugins
.map( ( item ) => getPluginConstructor( item ) )
Copy link
Member

Choose a reason for hiding this comment

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

It's not an item. It's a plugin name or constructor.

.filter( ( PluginConstructor ) => !!PluginConstructor );

// Check whether a length of passed array with plugins is the same like an array with mapped plugins.
Copy link
Member

Choose a reason for hiding this comment

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

As you noticed yourself, this comment needs to be refined :D

Copy link
Member

Choose a reason for hiding this comment

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

Besides, I don't like splitting the filtering which is done above from the check which is done below because it's a single piece of logic – can't find a plugin – throw.

OTOH, I can see that when using filter() it would be hard to reject the promise and stop the execution, so the check makes sense to be separated. So let's keep it like that, but please move it to a separate function. If you'll name this function intuitively, you won't need that comment :P.

The code in here could look like this:

const missingPlugins = getMissingPlugins( plugins, pluginConstructorsToLoad );

if ( missingPlugins ) {
    return Promise.reject();
}

if ( pluginConstructorsToLoad.length !== plugins.length ) {
// Extract names of plugins which cannot be loaded.
const notFoundPlugins = plugins.filter( ( item ) => !pluginConstructorsToLoad.includes( item ) );
Copy link
Member

Choose a reason for hiding this comment

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

I think that this is a wrong way of getting missing plugins. In plugins you may have strings and in pluginConstructorsToLoad you will have constructors. So checking if the former is in the latter doesn't make sense. Use getPluginConstructor() again.


log.error( 'plugincollection-load: It was not possible to load the plugins.', { plugins: notFoundPlugins } );

/**
* The plugins 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.
*
* @error plugincollection-plugin-not-found
* @param {Array.<String>} plugins The name of the plugins which could not be loaded.
*/
return Promise.reject( new CKEditorError(
'plugincollection-plugin-not-found: The plugin cannot be loaded by name.',
{ plugins: notFoundPlugins }
) );

// TODO update this error with links to docs because it will be a frequent problem.
}

return arr;
}, [] );
// Plugins that will be removed can be the constructors or names. We need to transform plugin names to plugin constructors.
const pluginConstructorsToRemove = removePlugins.map( ( item ) => getPluginConstructor( item ) )
.filter( ( PluginConstructor ) => !!PluginConstructor );

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

function loadPlugin( PluginConstructorOrName ) {
function loadPlugin( PluginConstructor ) {
// Don't load the plugin if it cannot be loaded.
Copy link
Member

Choose a reason for hiding this comment

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

This comment is unnecessary.

if ( removePlugins.includes( PluginConstructorOrName ) ) {
if ( pluginConstructorsToRemove.includes( PluginConstructor ) ) {
return;
}

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

return instantiatePlugin( PluginConstructorOrName )
return instantiatePlugin( PluginConstructor )
.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: PluginConstructorOrName } );
log.error( 'plugincollection-load: It was not possible to load the plugin.', { plugin: PluginConstructor } );

throw err;
} );
}

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

loading.add( PluginConstructor );

assertIsPlugin( PluginConstructor );

if ( PluginConstructor.requires ) {
PluginConstructor.requires.forEach( ( RequiredPluginConstructorOrName ) => {
if ( removePlugins.includes( RequiredPluginConstructorOrName ) ) {
const RequiredPluginConstructor = getPluginConstructor( RequiredPluginConstructorOrName );

if ( !RequiredPluginConstructor ) {
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 unnecessary in this PR. It doesn't make sense for a to require other plugins by their names.

Copy link
Member

Choose a reason for hiding this comment

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

PS. I meant that this may be a possible feature in the future, but it's not part of the ticket for which you're making a PR, so it shouldn't be included in it.

/**
* The plugin cannot be loaded by name.
*
* @error plugincollection-plugin-not-found
* @param {String} plugin The name of the plugin which could not be loaded.
*/
throw new CKEditorError(
'plugincollection-plugin-not-found: The plugin cannot be loaded by name.',
{ plugins: RequiredPluginConstructorOrName }
);
}

if ( removePlugins.includes( RequiredPluginConstructor ) ) {
/**
* Cannot load a plugin because one of its dependencies is
* listed in the `removePlugins` options.
* Cannot load a plugin because one of its dependencies is listed in the `removePlugins` options.
*
* @error plugincollection-required
* @param {*} plugin The required plugin.
* @param {*} requiredBy The parent plugin.
* @param {Function} plugin The required plugin.
* @param {Function} requiredBy The parent plugin.
*/
throw new CKEditorError(
'plugincollection-required: Cannot load a plugin because one of its dependencies is listed in' +
'the `removePlugins` options.',
{ plugin: RequiredPluginConstructorOrName, requiredBy: PluginConstructorOrName }
{ plugin: RequiredPluginConstructor, requiredBy: PluginConstructor }
);
}

loadPlugin( RequiredPluginConstructorOrName );
loadPlugin( RequiredPluginConstructor );
} );
}

Expand All @@ -179,27 +210,7 @@ export default class PluginCollection {
return PluginConstructorOrName;
}

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

if ( !PluginConstructor ) {
/**
* 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.
*
* @error plugincollection-plugin-not-found
* @param {String} pluginName The name of the plugin which could not be loaded.
*/
throw new CKEditorError(
'plugincollection-plugin-not-found: The plugin cannot be loaded by name.',
{ plugin: PluginConstructorOrName }
);

// TODO update this error with links to docs because it will be a frequent problem.
}

return PluginConstructor;
return that._availablePlugins.get( PluginConstructorOrName );
}

function assertIsPlugin( PluginConstructor ) {
Expand Down
21 changes: 21 additions & 0 deletions tests/plugincollection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,27 @@ describe( 'PluginCollection', () => {
expect( logSpy.calledTwice ).to.equal( true );
} );
} );

it( 'should reject when loaded plugin requires non-existing plugin', () => {
let logSpy = testUtils.sinon.stub( log, 'error' );

let plugins = new PluginCollection( editor, availablePlugins );

PluginA.requires = [ 'NonExistentPlugin' ];

return plugins.load( [ PluginA ] )
// 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 ).to.be.an.instanceof( CKEditorError );
expect( err.message ).to.match( /^plugincollection-plugin-not-found/ );

sinon.assert.calledOnce( logSpy );
expect( logSpy.args[ 0 ][ 0 ] ).to.match( /^plugincollection-load:/ );
} );
} );
} );

describe( 'get()', () => {
Expand Down