ckeditor / ckeditor5-design

☠ Early discussions about CKEditor 5's architecture. Closed now. Go to https://github.com/ckeditor/ckeditor5 ☠
58 stars 12 forks source link

Application architecture – editor should be subclassable #140

Closed Reinmar closed 7 years ago

Reinmar commented 8 years ago

Currently the CKEDITOR.create() method creates an instance of Editor class. The Editor loads its plugins (creator is one of them), instantiate and initialises them. After all is ready the Creator#create method is called which should finish the creation.

This idea that the above is wrong was always somewhere there, but I've never saw real problems with our current approach. There are even some nice outcomes of the above scheme – e.g. the creator can require some plugins and all is handled by a single plugin collection.

However, more doubts started appearing when we begun implementing real creator, with the whole data model and UI. It turns out that a creator needs to... create 90% of the editor instance. It initialises document, pipelines, editables and UI for all that, but it can do that in its constructor the earliest, when other plugins (e.g. some features) could already be instantiated and know about the editor.

We could change the loading order so the creator is instantiated before any other code, but then we'd violate the fact that plugins required by the creator should be instantiated before it. We can go further and remove that rule or live with it of course, but that doesn't sound right.

Another case is the Editor#setData() method. How can the editor know how it will be defined (all its editables and relations between them)? Wouldn't it make sense if the creator defined this method? It certainly would if it could. In JS it's of course possible, but this is ugly as well.

So perhaps the problem is somewhere else?

The creator defines the editor. It specialises the generic Editor class to do some specific things – create a specific UI, data processor, editables, etc. So it seems like a perfect case for inheritance. Let the editor create itself.

We discussed this internally already and I see that the idea sells itself. It seems to be right that either you pass editor class of your choice to CKEDITOR.create() or that perhaps you even instantiate it yourself (why CKEDITOR in the first place?).

There are some buts, though.

I wanted to write that I don't want to work on this refactoring now because we need to advance with gluing all the pieces that we created. However, after figuring out that it's actually possible to extend editor this way now, I lost the interest in making any real change other than ensuring that you can inherit from the editor. So there are couple of things that we should take into consideration:

... And perhaps more. All for good. And we can do all without doing any bigger architectural changes.

pjasiun commented 8 years ago

More I think about the creators, more I feel that we try to reinvent the wheel. Prepare a way to create object and let users extend the base functionality. There is such mechanism in the language, it is exactly what inheritance is about.

When I see:

editor.ui = this._createEditorUI();
editor.document = new Document();
editor.editing = new EditingController( editor.document );
editor.data = new DataController( editor.document, new HtmlDataProcessor() );

I have a feeling that this is exactly what constructor should do.

Yesterday we where thinking who should describe a special behaviour for getData/setData and all solutions looked bad. With the inheritance the child class could overwrite these methods.

class MultipleEditableEditor extends Editor {
    // ...
    getData() {
        return '<h1>' + this.data.get( 'headed' ) + '</h1><article>' + this.data.get( 'article' ) + '</article>';
    }
}

The base class should define the API, which is required by features, and as long a custom editor follow that API, all plugins should work.

Then it is very simple to create editor:

import InlineEditor from './editor.js';

const editor = new InlineEditor( config );

What with CKEDITOR? It could be a singleton. Base Editor do:

import CKEDITOR from './ckeditor.js';

class Editor {
    constructor() {
        CKEDITOR.register( this );
    }
}

Also, in fact, creator is not a plugin. I can not use other plugins, because it have to be loaded before them anyway.

Reinmar commented 8 years ago

I've been thinking about one related topic – how to implement the Editor so it can be used in Node.js. I've got some basic DOM-related logic which I'll try to keep outside for now (in CKEDITOR.create() presumably). Later on we can decide to implement BaseEditor class which would be enough for Node.js and Editor inheriting from it which would keep the additional logic.

Reinmar commented 8 years ago

There's a related discussion in https://github.com/ckeditor/ckeditor5/issues/178

pjasiun commented 8 years ago

Also now we use some hacks to make Editor fit all needs. We introduced getters like firstElement and firstElementName (https://github.com/ckeditor/ckeditor5/blob/master/src/editor.js#L153), because in most cases editor will have only one root editable element. It would be much cleaner if we would have a separate base editor for a single editable editor, so instead of collection of editable roots it will handle only one.

Reinmar commented 8 years ago

Yep, we can make the editor classes cleaner if we split them. Because there will be no need for a one uber class which implements all possible tools.

Reinmar commented 8 years ago

We decided to do this move. See ckeditor/ckeditor5-engine#185 for more info.

Reinmar commented 7 years ago

This has been implemented – Editor is a normal class and can be easily subclassed. Although, with time we also understood that subclassing is to be avoided as much as possible in such cases and we'll be doing a small refactoring soon to limit the subclassing to max 1 level.