From 98f009d3e181b3bde52b48667104c692dcee4445 Mon Sep 17 00:00:00 2001 From: Kamil Piechaczek Date: Mon, 29 May 2017 10:46:38 +0200 Subject: [PATCH 1/8] Improvements for Plugin, PluginCollection and Editor classes. See details. Added default implementation for `Plugin.destroy()`. Introduced `PluginCollection.destroy()` method which calls `Plugin.destroy()` for each loaded plugin. `Editor.destroy()` calls `PluginCollection.destroy()` method too. --- src/editor/editor.js | 1 + src/plugin.js | 1 + src/plugincollection.js | 15 +++++++++++++++ tests/editor/editor-base.js | 2 ++ tests/plugin.js | 9 +++++++++ tests/plugincollection.js | 20 ++++++++++++++++++++ 6 files changed, 48 insertions(+) diff --git a/src/editor/editor.js b/src/editor/editor.js index f4d6349c..ba1b0c51 100644 --- a/src/editor/editor.js +++ b/src/editor/editor.js @@ -149,6 +149,7 @@ export default class Editor { return Promise.resolve() .then( () => { + this.plugins.destroy(); this.document.destroy(); this.data.destroy(); } ); diff --git a/src/plugin.js b/src/plugin.js index 29a5cc17..0e0577dc 100644 --- a/src/plugin.js +++ b/src/plugin.js @@ -34,6 +34,7 @@ export default class Plugin { * @inheritDoc */ destroy() { + this.stopListening(); } } diff --git a/src/plugincollection.js b/src/plugincollection.js index a894d41c..9653f295 100644 --- a/src/plugincollection.js +++ b/src/plugincollection.js @@ -207,6 +207,21 @@ export default class PluginCollection { } } + /** + * Destroys each loaded plugin. + * + * @returns {Promise} + */ + destroy() { + const promises = []; + + for ( const [ , pluginInstance ] of this ) { + promises.push( pluginInstance.destroy() ); + } + + return Promise.all( promises ); + } + /** * Adds the plugin to the collection. Exposed mainly for testing purposes. * diff --git a/tests/editor/editor-base.js b/tests/editor/editor-base.js index 60c5a84c..2e93487e 100644 --- a/tests/editor/editor-base.js +++ b/tests/editor/editor-base.js @@ -41,11 +41,13 @@ describe( 'Editor', () => { const spy1 = sinon.spy( editor.data, 'destroy' ); const spy2 = sinon.spy( editor.document, 'destroy' ); + const spy3 = sinon.spy( editor.plugins, 'destroy' ); return editor.destroy() .then( () => { expect( spy1.calledOnce ).to.be.true; expect( spy2.calledOnce ).to.be.true; + expect( spy3.calledOnce ).to.be.true; } ); } ); } ); diff --git a/tests/plugin.js b/tests/plugin.js index d2fa73df..88f68216 100644 --- a/tests/plugin.js +++ b/tests/plugin.js @@ -25,5 +25,14 @@ describe( 'constructor()', () => { expect( plugin.destroy ).to.be.a( 'function' ); } ); + + it( 'should stop listening', () => { + const plugin = new Plugin( editor ); + const stopListeningSpy = sinon.spy( plugin, 'stopListening' ); + + plugin.destroy(); + + expect( stopListeningSpy.calledOnce ).to.equal( true ); + } ); } ); } ); diff --git a/tests/plugincollection.js b/tests/plugincollection.js index 74c63663..481a38bb 100644 --- a/tests/plugincollection.js +++ b/tests/plugincollection.js @@ -381,6 +381,26 @@ describe( 'PluginCollection', () => { } ); } ); + describe( 'destroy()', () => { + it( 'calls "destroy" method on each loaded plugin', () => { + let destroySpyForPluginA, destroySpyForPluginB; + + const plugins = new PluginCollection( editor, [] ); + + return plugins.load( [ PluginA, PluginB ] ) + .then( () => { + destroySpyForPluginA = testUtils.sinon.spy( plugins.get( PluginA ), 'destroy' ); + destroySpyForPluginB = testUtils.sinon.spy( plugins.get( PluginB ), 'destroy' ); + + return plugins.destroy(); + } ) + .then( () => { + expect( destroySpyForPluginA.calledOnce ).to.equal( true ); + expect( destroySpyForPluginB.calledOnce ).to.equal( true ); + } ); + } ); + } ); + describe( 'iterator', () => { it( 'exists', () => { const plugins = new PluginCollection( editor, availablePlugins ); From 6fcfc40b58f1084387f1c767eed3a8078d27d6b1 Mon Sep 17 00:00:00 2001 From: Kamil Piechaczek Date: Mon, 29 May 2017 11:56:51 +0200 Subject: [PATCH 2/8] Editor.destroy - plugins will be destroyed first. --- src/editor/editor.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/editor/editor.js b/src/editor/editor.js index ba1b0c51..bd290fef 100644 --- a/src/editor/editor.js +++ b/src/editor/editor.js @@ -147,9 +147,8 @@ export default class Editor { this.fire( 'destroy' ); this.stopListening(); - return Promise.resolve() + return this.plugins.destroy() .then( () => { - this.plugins.destroy(); this.document.destroy(); this.data.destroy(); } ); From 0a9dadddfb79e56b36129109a0371db67d4adacf Mon Sep 17 00:00:00 2001 From: Kamil Piechaczek Date: Mon, 29 May 2017 11:57:26 +0200 Subject: [PATCH 3/8] Updated docs for PluginCollection.destroy(). --- src/plugincollection.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugincollection.js b/src/plugincollection.js index 9653f295..d47b4640 100644 --- a/src/plugincollection.js +++ b/src/plugincollection.js @@ -208,7 +208,7 @@ export default class PluginCollection { } /** - * Destroys each loaded plugin. + * Destroys all loaded plugins. * * @returns {Promise} */ From a91b9c8e6b985e170504f0d0eb11c73d9d60a58b Mon Sep 17 00:00:00 2001 From: Kamil Piechaczek Date: Mon, 29 May 2017 14:02:23 +0200 Subject: [PATCH 4/8] Fix: PluginCollection.destroy() does not crash if a plugin does not have a "destroy()" method. --- src/plugincollection.js | 4 ++- tests/plugincollection.js | 59 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/src/plugincollection.js b/src/plugincollection.js index d47b4640..4dc6ce37 100644 --- a/src/plugincollection.js +++ b/src/plugincollection.js @@ -216,7 +216,9 @@ export default class PluginCollection { const promises = []; for ( const [ , pluginInstance ] of this ) { - promises.push( pluginInstance.destroy() ); + if ( typeof pluginInstance.destroy == 'function' ) { + promises.push( pluginInstance.destroy() ); + } } return Promise.all( promises ); diff --git a/tests/plugincollection.js b/tests/plugincollection.js index 481a38bb..194b69cf 100644 --- a/tests/plugincollection.js +++ b/tests/plugincollection.js @@ -3,6 +3,8 @@ * For licensing, see LICENSE.md. */ +/* globals setTimeout */ + import testUtils from '../tests/_utils/utils'; import Editor from '../src/editor/editor'; import PluginCollection from '../src/plugincollection'; @@ -389,8 +391,8 @@ describe( 'PluginCollection', () => { return plugins.load( [ PluginA, PluginB ] ) .then( () => { - destroySpyForPluginA = testUtils.sinon.spy( plugins.get( PluginA ), 'destroy' ); - destroySpyForPluginB = testUtils.sinon.spy( plugins.get( PluginB ), 'destroy' ); + destroySpyForPluginA = sinon.spy( plugins.get( PluginA ), 'destroy' ); + destroySpyForPluginB = sinon.spy( plugins.get( PluginB ), 'destroy' ); return plugins.destroy(); } ) @@ -399,6 +401,59 @@ describe( 'PluginCollection', () => { expect( destroySpyForPluginB.calledOnce ).to.equal( true ); } ); } ); + + it( 'waits when all plugins are destroyed', () => { + const destroyedPlugins = []; + + class AsynchronousPluginA extends Plugin { + destroy() { + return new Promise( resolve => { + setTimeout( () => { + super.destroy(); + destroyedPlugins.push( 'AsynchronousPluginA.destroy()' ); + resolve(); + }, 200 ); + } ); + } + } + + class AsynchronousPluginB extends Plugin { + destroy() { + return new Promise( resolve => { + setTimeout( () => { + super.destroy(); + destroyedPlugins.push( 'AsynchronousPluginB.destroy()' ); + resolve(); + }, 100 ); + } ); + } + } + + const plugins = new PluginCollection( editor, [] ); + + return plugins.load( [ AsynchronousPluginA, AsynchronousPluginB ] ) + .then( () => plugins.destroy() ) + .then( () => { + expect( destroyedPlugins ).to.contain( 'AsynchronousPluginB.destroy()' ); + expect( destroyedPlugins ).to.contain( 'AsynchronousPluginA.destroy()' ); + } ); + } ); + + it( 'does not execute "Plugin.destroy()" for plugins which do not have this method', () => { + class FooPlugin { + constructor( editor ) { + this.editor = editor; + } + } + + const plugins = new PluginCollection( editor, [] ); + + return plugins.load( [ PluginA, FooPlugin ] ) + .then( () => plugins.destroy() ) + .then( destroyedPlugins => { + expect( destroyedPlugins.length ).to.equal( 1 ); + } ); + } ); } ); describe( 'iterator', () => { From d7165a7e7ff6204aef23a73d396c3ec528952acd Mon Sep 17 00:00:00 2001 From: Kamil Piechaczek Date: Mon, 29 May 2017 14:07:07 +0200 Subject: [PATCH 5/8] Implementation for PluginCollection.destroy() is more readable. --- src/plugincollection.js | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/plugincollection.js b/src/plugincollection.js index 4dc6ce37..df3a5567 100644 --- a/src/plugincollection.js +++ b/src/plugincollection.js @@ -213,13 +213,10 @@ export default class PluginCollection { * @returns {Promise} */ destroy() { - const promises = []; - - for ( const [ , pluginInstance ] of this ) { - if ( typeof pluginInstance.destroy == 'function' ) { - promises.push( pluginInstance.destroy() ); - } - } + const promises = Array.from( this ) + .map( ( [ , pluginInstance ] ) => pluginInstance ) + .filter( pluginInstance => typeof pluginInstance.destroy == 'function' ) + .map( pluginInstance => pluginInstance.destroy() ); return Promise.all( promises ); } From 6e9d21c7ee64d3ae2f91c98612f41ad93e393a03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Mon, 29 May 2017 16:48:15 +0200 Subject: [PATCH 6/8] Minor improvements in the tests. --- tests/plugincollection.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/tests/plugincollection.js b/tests/plugincollection.js index 194b69cf..9064d40f 100644 --- a/tests/plugincollection.js +++ b/tests/plugincollection.js @@ -384,7 +384,7 @@ describe( 'PluginCollection', () => { } ); describe( 'destroy()', () => { - it( 'calls "destroy" method on each loaded plugin', () => { + it( 'calls Plugin#destroy() method on every loaded plugin', () => { let destroySpyForPluginA, destroySpyForPluginB; const plugins = new PluginCollection( editor, [] ); @@ -402,7 +402,7 @@ describe( 'PluginCollection', () => { } ); } ); - it( 'waits when all plugins are destroyed', () => { + it( 'waits until all plugins are destroyed', () => { const destroyedPlugins = []; class AsynchronousPluginA extends Plugin { @@ -410,9 +410,10 @@ describe( 'PluginCollection', () => { return new Promise( resolve => { setTimeout( () => { super.destroy(); + destroyedPlugins.push( 'AsynchronousPluginA.destroy()' ); resolve(); - }, 200 ); + } ); } ); } } @@ -422,9 +423,10 @@ describe( 'PluginCollection', () => { return new Promise( resolve => { setTimeout( () => { super.destroy(); + destroyedPlugins.push( 'AsynchronousPluginB.destroy()' ); resolve(); - }, 100 ); + } ); } ); } } @@ -439,7 +441,7 @@ describe( 'PluginCollection', () => { } ); } ); - it( 'does not execute "Plugin.destroy()" for plugins which do not have this method', () => { + it( 'does not execute Plugin#destroy() for plugins which do not have this method', () => { class FooPlugin { constructor( editor ) { this.editor = editor; From c5e53090aa83a6f0b6ce2492ec82c5d5630ad6eb Mon Sep 17 00:00:00 2001 From: Kamil Piechaczek Date: Wed, 31 May 2017 12:51:55 +0200 Subject: [PATCH 7/8] Changed order of destroying the editor layers. Plugins must be destroyed before the UI. --- tests/_utils/classictesteditor.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/_utils/classictesteditor.js b/tests/_utils/classictesteditor.js index d8c864ec..271edad2 100644 --- a/tests/_utils/classictesteditor.js +++ b/tests/_utils/classictesteditor.js @@ -43,8 +43,8 @@ export default class ClassicTestEditor extends StandardEditor { destroy() { this._elementReplacer.restore(); - return this.ui.destroy() - .then( () => super.destroy() ); + return super.destroy() + .then( () => this.ui.destroy() ); } /** From 40f33cfd59415e9a96ae6ad95053b0fb7aa892fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Tue, 20 Jun 2017 20:33:01 +0200 Subject: [PATCH 8/8] After merge fixes. --- tests/editor/editor-base.js | 76 ------------------------------------- tests/editor/editor.js | 2 + 2 files changed, 2 insertions(+), 76 deletions(-) delete mode 100644 tests/editor/editor-base.js diff --git a/tests/editor/editor-base.js b/tests/editor/editor-base.js deleted file mode 100644 index 2e93487e..00000000 --- a/tests/editor/editor-base.js +++ /dev/null @@ -1,76 +0,0 @@ -/** - * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved. - * For licensing, see LICENSE.md. - */ - -import Editor from '../../src/editor/editor'; -import Command from '../../src/command/command'; -import Locale from '@ckeditor/ckeditor5-utils/src/locale'; -import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; - -describe( 'Editor', () => { - describe( 'locale', () => { - it( 'is instantiated and t() is exposed', () => { - const editor = new Editor(); - - expect( editor.locale ).to.be.instanceof( Locale ); - expect( editor.t ).to.equal( editor.locale.t ); - } ); - - it( 'is configured with the config.lang', () => { - const editor = new Editor( { lang: 'pl' } ); - - expect( editor.locale.lang ).to.equal( 'pl' ); - } ); - } ); - - describe( 'destroy', () => { - it( 'should fire "destroy"', () => { - const editor = new Editor(); - const spy = sinon.spy(); - - editor.on( 'destroy', spy ); - - return editor.destroy().then( () => { - expect( spy.calledOnce ).to.be.true; - } ); - } ); - - it( 'should destroy all components it initialized', () => { - const editor = new Editor(); - - const spy1 = sinon.spy( editor.data, 'destroy' ); - const spy2 = sinon.spy( editor.document, 'destroy' ); - const spy3 = sinon.spy( editor.plugins, 'destroy' ); - - return editor.destroy() - .then( () => { - expect( spy1.calledOnce ).to.be.true; - expect( spy2.calledOnce ).to.be.true; - expect( spy3.calledOnce ).to.be.true; - } ); - } ); - } ); - - describe( 'execute', () => { - it( 'should execute specified command', () => { - const editor = new Editor(); - - const command = new Command( editor ); - sinon.spy( command, '_execute' ); - - editor.commands.set( 'commandName', command ); - editor.execute( 'commandName' ); - - expect( command._execute.calledOnce ).to.be.true; - } ); - - it( 'should throw an error if specified command has not been added', () => { - const editor = new Editor(); - - expect( () => { - editor.execute( 'command' ); - } ).to.throw( CKEditorError, /^editor-command-not-found:/ ); - } ); - } ); -} ); diff --git a/tests/editor/editor.js b/tests/editor/editor.js index 94caa778..f25ad82b 100644 --- a/tests/editor/editor.js +++ b/tests/editor/editor.js @@ -175,11 +175,13 @@ describe( 'Editor', () => { const spy1 = sinon.spy( editor.data, 'destroy' ); const spy2 = sinon.spy( editor.document, 'destroy' ); + const spy3 = sinon.spy( editor.plugins, 'destroy' ); return editor.destroy() .then( () => { expect( spy1.calledOnce ).to.be.true; expect( spy2.calledOnce ).to.be.true; + expect( spy3.calledOnce ).to.be.true; } ); } ); } );