Ckeditor5: Make Plugin an interface

[Reinmar]

I think we agreed that PluginCollection can easily become a util. For that we have https://github.com/ckeditor/ckeditor5-core/issues/57.

The second part of the puzzle is to remove the Plugin class and make it an interface. Interface which defines that requires and pluginName static props have special meaning and that the constructor is called with a plugin's context (which is different, depending who's the owner of the plugin collection).

This will allow us to remove ckeditor5-core imports from many packages and will allow defining plugins on the fly. E.g. like this:

function enableMarkdownDP( editor ) {
    editor.data.processor = new MarkdownDP();
}

ClassicEditor.create( el, {
    plugins: [ ArticlePreset, enableMarkdownDP ]
} )
...

Yep, class constructor is just a function, so you don't even have to use ES6 classes here.

[oleq]

The second part of the puzzle is to remove the Plugin class and make it an interface.

I would definitely help us get rid of core deps in the UI like this one https://github.com/ckeditor/ckeditor5-ui/blob/master/src/contextualballoon.js#L30.

What are disadvantages of the interface approach? I struggle to find any but, for some reason, we decided to go with the Plugin class in the first place. I just don't remember why ;-)

[Reinmar]

What are disadvantages of the interface approach? I struggle to find any but, for some reason, we decided to go with the Plugin class in the first place. I just don't remember why ;-)

Pretty much none. That you won't find immediately that you have a broken installation of CKE packages. Right now you get an error that you try to load something which is not an instance of a plugin and this happens if some pkg is duplicated. It's both annoying and super helpful because it catches the issue very early.

Other than that, I had this idea to have plugin as an interface for a very long time and never had much against it, so there were no other reasons. The initial implementation of the plugin system defined plugin as a class and then we followed.

[oleq]

That you won't find immediately that you have a broken installation of CKE packages. Right now you get an error that you try to load something which is not an instance of a plugin and this happens if some pkg is duplicated. It's both annoying and super helpful because it catches the issue very early.

I guess we can preserve this making the pluginName a mandatory property of the interface, right?

[Reinmar]

I guess we can preserve this making the pluginName a mandatory property of the interface, right?

Nope. It's just a property. It doesn't check whether the core package isn't duplicated somewhere.

[Reinmar]

OK, the plan of action here needs to make sure that we'll not be paralyzed during the process, so I propose to split it into 3 steps:

1. Allow defining plugins which are simple functions

This is a change which needs to be done here, in this repo. PluginCollection needs to stop checking whether the passed plugin is an instance of Plugin.

We should move the documentation of Plugin class to the plugincollection.js and document it as an interface, not a class.

All tests in this repo should stop using the Plugin class.

We should add a warning directly in the plugin.js file that usage of this module is deprecated.

2. Converting existing plugins to simple functions

After step 1. is done and merged, we need to convert existing plugins (all across our codebase).

Most of the time it's just a matter of removing extends Plugin, removing the import, fixing the constructor so it handles the editor param itself (sets it to this.editor) and changing @extends Plugin to @implements Plugin (and fixing path because the Plugin interface is now defined in a different module).

3. When no plugins left

After we have all existing code changed and no warnings about use of the deprecated module in the tests, we can remove the plugin.js module completely.

[Reinmar]

There will be a bit more work in step 1. because the destroy(), afterInit() etc methods will now be optional. They don't need to be implemented by a class implementing the Plugin interface. This means changes in the PluginCollection and in the docs for Plugin.

[Reinmar]

I've just realised that this is a bad idea ;|

Plugin is a handful util which implements the observable mixin and exposes destroy() method which should call this.stopListening() (it doesn't do it yet).

If we'll remove this class, we'll force everyone to mix the observable API and implement destroy().

Sorry for the mess.

I need to yet think on whether we shouldn't do anything or just loosen the instanceof Plugin check to accept any functions.

[Reinmar]

OK, I guess it's quite useful if you were able to pass any function as a plugin. This may be especially helpful if you're dealing with a built editor and don't have access to the Plugin class.

In such case, we only need to split PluginInterface from Plugin (and make Plugin implement PluginInterface). We also need to remove the instanceof check.

This means far less work because we won't be touching existing plugins. It's only ckeditor5-core related change.

[pomek]

Sounds good. I will fix the PR.

[Reinmar]

I guess you should just start from scratch. Don't mix two different concepts in one PR.