T/86: Plugin#destroy() should remove listeners #87
Conversation
…ails. 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
Outdated
| @@ -149,6 +149,7 @@ export default class Editor { | |||
|
|
|||
| return Promise.resolve() | |||
| .then( () => { | |||
| this.plugins.destroy(); | |||
There was a problem hiding this comment.
You made it return a promise, but you don't handle this promise. Which means there's no good test for it.
src/plugincollection.js
Outdated
| @@ -208,6 +208,21 @@ export default class PluginCollection { | |||
| } | |||
|
|
|||
| /** | |||
| * Destroys each loaded plugin. | |||
|
@Reinmar, could you review once again? I am not sure whether I did what you wanted. |
|
There's still no test what |
src/plugincollection.js
Outdated
| destroy() { | ||
| const promises = []; | ||
|
|
||
| for ( const [ , pluginInstance ] of this ) { |
There was a problem hiding this comment.
First of all: this could be simplified as: Array.from( this._plugins.values() ).map( plugin => plugin.destroy() )
Second, destroy() is optional in PluginInterface.
There was a problem hiding this comment.
We cannot use Array.from( this._plugins.values() ) because we will call plugin.destroy() twice for plugins that have a defined name.
There was a problem hiding this comment.
But we can still do Array.from( this ).map( ... ).
…ave a "destroy()" method.
|
@Reinmar, I added tests async plugins and plugin which does not have a |
|
All fine now, except the fact that tests started failing. Run all of them and you'll see at least couple failed. |
…yed before the UI.
|
Thanks to @szymonkups we figured out what happens with the failing tests. UI of the editor was removed too early and the plugins cannot destroy themselves. Plugins must be destroyed before destroying the UI. But unfortunately, one test still fails. In order to reproduce the bug, you need to check out Anyone could help me? |
|
return this.ui.destroy()
.then( () => super.destroy() );
|
I'm not sure that I understand – I think you missed explaining the crucial bit. Why plugins can't destroy themselves if the UI is removed? Based on @szymonkups's comment I'm guessing that in that particular test UI isn't destroyed before being removed and that's causing some kind of issue. Is that true? |
You know that I cannot know the answer to this question… It looks like two different things use the same object (let's call it as But I figured out another an interesting thing. The failing tests ( So at this moment, I found two solutions which will resolve the issue with failing tests:
I don't know which solution is proper… |
|
I'm not sure if that helps, but we faced similar issues in the past, like https://github.com/ckeditor/ckeditor5-ui/issues/203. ATM, the async destruction chain when
It's all recursive and Promise–controlled, so the code destroying the root view must respect that. It's enough to destroy the root view to kill the entire tree so you got to make sure things aren't called twice or sth. TBH, it all boils down to https://github.com/ckeditor/ckeditor5-ui/issues/225, which will be such a relief for us and I'm really looking forward to seeing it happen. |
|
Hm... the very first thing which is confusing here is which tests fail exactly. E.g. I started investigating this from the failing it.only( 'should remove balloon panel view from editor body collection and clear stack', () => {
balloon.destroy();
expect( editor.ui.view.body.getIndex( balloon.view ) ).to.equal( -1 );
expect( balloon.visibleView ).to.null;
} );And the destroy() {
this.editor.ui.view.body.remove( this.view );
this.view.destroy();
this._stack.clear();
super.destroy();
}How do you expect this test to work if the |
|
Are there any other tests failing? |
No. Everything except this one ( |
|
Are you sure? I've just run the tests and got this: |
|
I said you need to check out one more branch.
|
|
OK, sorry. I haven't noticed that part of your comment. |
|
I've looked into this and I think that we're facing here a more general issue. I'm 99% sure that the change proposed in ckeditor5-editor-classic is incorrect. I wrote more about this topic in ckeditor/ckeditor5#114 (comment). Without figuring this out we won't be able to close this PR. |
|
This PR is blocked by https://github.com/ckeditor/ckeditor5-ui/issues/248. |
Please check if ckeditor/ckeditor5-ui#254 helps. |
|
All tests pass with ckeditor/ckeditor5-ui#254. |
Suggested merge commit message (convention)
Feature: Added default implementation for
Plugin.destroy(). IntroducedPluginCollection.destroy()method which callsPlugin.destroy()for each loaded plugin.Editor.destroy()callsPluginCollection.destroy()method too. Closes ckeditor/ckeditor5#2916.