Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI libraries architecture #133

Closed
Reinmar opened this issue Feb 16, 2016 · 15 comments
Closed

UI libraries architecture #133

Reinmar opened this issue Feb 16, 2016 · 15 comments

Comments

@Reinmar
Copy link
Member

Reinmar commented Feb 16, 2016

It's high time to discuss how the UI libraries will work in CKEditor 5. All we know so far is that the core UI library will be a simple implementation of the MVC pattern what will decouple views from the rest of the code. This will satisfy (from the code perspective) the most important requirement which is that developers will be able to replace the default UI library with their implementations.

I want to approach the design of this architecture based on two most important use cases: the toolbar case and typical UI components like autocompleter or dialogs. Those cases are different in this sense, that in the former one the feature does not instantiate the buttons by itself, it only registers them, while in the latter the feature does all the job.

Specialisation

Layers of specialisation:

  • The core (or base) UI library. Simple implementation, not yet specialised for the editor. The rule of thumb should be that it should be reusable in other projects. I think that we want to keep this in the ckeditor5-core repository.
  • The default UI library. CKEditor's default UI library. Based on the core UI library, specialised for the editor. It will also implement the theming mechanism. It will be kept in a separate package (most likely ckeditor5-ui-default).
  • Replacements or extensions for the default UI library. Packages which will implement pieces of the default UI library or reimplement it entirely.

Requirements

Requirements for the UI libraries architecture:

  • It needs to allow easily generate optimised bundles. Initially we've been considering some repository of components, but that would break the bundles optimisation as all the UI components would seem to be required (by the repo). Therefore, the features need to import the components which they require by hand.
  • The feature registering some button won't know how many instances of that component will be needed (e.g. some creator may want to create two toolbars). This means that we'll need a factory of those components.
    • It wasn't clear initially for which components we need it (dialogs? toolbars?), but at the moment I'd say that only for "feature representations", because other components will be instantiated by features using them directly (e.g. some mentions feature will instantiate the autocompleter component).
    • Since there may be multiple instances of one feature's component, there needs to be a single entry point through which the feature will be able to communicate will all these instances. It seems to be clear that this is the component's model (only a single instance, shared by all instances of the view). This clarifies that the component will need to fire action events on its model (we had some doubts about this).
    • The fact that all feature's representation components (registered under one name, because features could create more components) share one model clarifies that things like toolbar cannot be created this way (because you may want to have two different toolbars).

Code Split

Let's also look at the picture from a different perspective – where does the code of the MVC triad of a UI component come from?

  • View – UI library.
    • The views come from the chosen UI library. They only communicate with the model, so they are totally decoupled from the editor, hence the UI library is able to provide all the code.
  • Model – features + (UI lib controllers / UI lib models).
    • Features will directly define and control a great part of models state.
    • Depending on whether we'll want the models to be dry or not, they will be provided with some logic or that logic will come from their controllers (provided by the UI lib). We'll see how the fact that the models are the only interface of the components will drive the decision, but I presume that more will be placed in models. If a feature operates directly on the model those operations (state changes) may need to be controlled so the model could do it by itself.
    • They are the interface of the components, so big piece of logic comes from the features which play the role of controllers (so it's actually outside the models, but it directly operates on the models).
  • Controller – UI library (default + chosen).
    • Some editor-independent controller logic (bindings) can be generalised in the core UI library. The best way to share it is through component controllers.
    • Other bindings logic is related to the editor but it also can be shared, so it will come from the default UI library (with possible extensions in other UI libs).

Implementation

The first requirement, which is that the UI library must not break optimised builds means that features must directly import UI components' views, controllers and models (if provided).

Features need to use some predictable paths to the UI library, so whichever library the developer chose, the path must be the same. Therefore, we propose that the chosen library is copied by the builder to the dist/<format>/ui/ directory (make sure to read the Building article).

Now, that library (e.g. ckeditor5-ui-my) needs to be able to import modules from the default library (ckeditor5-ui-default). This means that its code must also be available in the dist/<format>/ directory. It's easiest to simply have it under its usual directory which would be dist/<format>/ui-default/.

And the last fact – the chosen UI lib may not implement the whole UI library, but just a couple components, just the views, or something. The features must still be able to import the missing pieces, so they need to be available in dist/<format>/ui/ as well.

Hence, the algorithm of the builder:

  1. Do the usual. The ckeditor5-ui-default package will land in ui-default/ and ckeditor5-ui-my in ui-my/
  2. Copy the default library from ui-default/ to ui/ directory.
  3. Copy the chosen UI library (if different than the default) to ui/ directory (override existing files).

Now the feature's code:

 // The controller.
import Button from '../ui/button/button.js';
import ButtonView from '../ui/button/buttonview.js';
// Let's assume that the button doesn't have a specialised model class.
import Model from '../core/model.js';

export default class Bold {
    constructor() {
        const boldModel = new Model( { isActive: false, label: 'bold' } );

        editor.ui.featureComponents.register( 'bold', Button, ButtonView, boldModel );

        boldModel.isActive = true;
    }
}

Now, let's consider where will the button get it localized label. The label: 'bold' property of the model defines only the language string key. Its value will need to be taken e.g. from editor.lang.bold.

That will be done by the controller of the button. As mentioned in "Code Split", the default library will define this binding. This means that the feature components registry must pass the editor to the controller when instantiating it.

The toolbar (or a creator) will later get a bold button instance in the following way:

const boldButton = editor.ui.featureComponents.create( 'bold' );

Notes

  • The steps 2-3 of the building algorithm can be defined as an array of packages which need to be copied to ui/ one by one. This will allow to compose the UI library out of many packages.

  • Let's assume we have a ckeditor5-ui-autocompleter package which defines an additional UI component. Do we want to make it re-definable by other UI libraries? I think so, otherwise other UI libraries will not be able to affect it easily – forks of the original UI component will need to be created and somehow injected in place of the orignal component (in its directory, because features will look of it in a specific way). That could even work, but then the initial code would not be reusable (because its directory is already occupied).

    Therefore I think that it should work differently – the package can somehow notify the builder that it contains an UI component and it would be unpacked inside ui/<provided-name>/. Then, other UI libraries will be able to contain also an extension for it.

  • If a component will not provide a special model class, then this means that other UI lib extension will not be able to define the model either because features will not start using it automatically. I think though though that it makes a perfect sense because the model is the interface of the component so it cannot be changed.

  • The builder will copy some code to two directories (e.g. the default UI lib will be in ui/ and ui-default/). I initially thought that if the features will use the UI library only through ui/ then it's OK, because the duplicates are not used. But neither it is completely true, nor we can say that features are only allowed to use what's in ui/. On the other hand, this is a very specific case, so perhaps we can live with a small code duplication?

    Alternatively, what if we do all the operations virtually, by changing the import paths? This would be tricky, because the builder would need to check which UI lib contains which files, to know what's the most important version of a specific file. We would also lose the nice touch of the first solution which is that you can see all the code in your file system and even do some overrides manually. It should avoid any code duplication though.

Uh... Questions? :D

@scofalik
Copy link

The builder will copy some code to two directories (e.g. the default UI lib will be in ui/ and ui-default/). I initially thought that if the features will use the UI library only through ui/ then it's OK, because the duplicates are not used. But neither it is completely true, nor we can say that features are only allowed to use what's in ui/. On the other hand, this is a very specific case, so perhaps we can live with a small code duplication?

  1. Why we just can't say that features should use ui/ directory? Do you have any scenario on your mind where this rule would make something impossible to code?
  2. I remember that at some point we were discussing that builder is able to remove unused files. Is it still true? Would files from ui-default that are not imported be removed?
  3. I can live with some code duplication. I think this is cleaner and less confusing solution that the one you proposed later (with import path substitution).

@scofalik scofalik mentioned this issue Feb 17, 2016
@Reinmar
Copy link
Member Author

Reinmar commented Feb 17, 2016

Why we just can't say that features should use ui/ directory? Do you have any scenario on your mind where this rule would make something impossible to code?

It's an edge case, so after giving it more thought I'm about to ignore it :D. The case looks like this – since all the installed (as packages) UI libraries are available somewhere in the build directory (e.g. as dist/<format>/ui-my/) some special features may want to use them directly saying that: even if the developer chose a different UI library when building CKEditor, I know that I need the "ui-my" lib. In such case the feature will require e.g. ui-my/button/button.js which will extend the default ui-default/button/button.js (so it will import it). At the same time the toolbar will use the button from ui/button/button.js. So if the chosen UI lib is the ui-default one, then the button will be used twice under different names, so the bundler will not optimise it.

But as I said – it's a super edge case. And given that it's quite unlikely that there will be many UI libraries out there (people will rather focus on theming the default one), that becomes even less probable. Hence, I don't think it matters.

And yes – I think that the general rule should be that features use components from ui/.

I remember that at some point we were discussing that builder is able to remove unused files. Is it still true? Would files from ui-default that are not imported be removed?

That was a different issue – ckeditor/ckeditor5#50 – and it was about removing files which are versioned for each format (amd, cjs, esnext). Not related to bundling.

The builder, in general, does not care about which files will be used or not as it doesn't know this. The bundler will need to make this decision. So far it's easy – take ckeditor.js and the features (files) that the developer wants and go through all dependencies. Just this, because so far every dependency is kept as an import statement.

I want to keep these rule, so that's why I propose that the features will import the UI component that they want to use. This gives us a straight info – the developer wants this feature and this feature requires this UI component.

Note how easily this nice state could've been destroyed. We were considering having a repository of UI components. There would need to be a registry of them. The feature would then say "I want to create a 'button' type of component". Nice, right? But the registry of these components would need to import all them, so the bundler would always think that all components are in use.

I can live with some code duplication. I think this is cleaner and less confusing solution that the one you proposed later (with import path substitution).

I think that too. In general, we are only opening doors for other UI libraries, but we don't have to optimise every possible, strange scenario.

@scofalik
Copy link

even if the developer chose a different UI library when building CKEditor, I know that I need the "ui-my" lib.

I knew that's the case. This is kinda defeating the purpose of decoupled UI library. I don't think that any developer should ever decide that this is the only UI lib that should be used with my feature. If components for that feature are available in overriding UI lib, they should be used. Otherwise, it will lead to strange scenarios where some part of UI is different than other.

I think that forcing importing UI from ui/ is doing more good than harm.

@Reinmar
Copy link
Member Author

Reinmar commented Feb 19, 2016

I started implementing the component registry. So the place where features register their buttons and they can be instantiated in other places.

I have this:

export default class ComponentRepository {
    constructor( editor ) {
        /**
         * @readonly
         * @type {core.Editor}
         */
        this.editor = editor;

        this._components = new Map();
        this._instances = new Set();
    }

    add( name, ControllerClass, ViewClass, model ) {
        this._components.set( name, {
            ControllerClass,
            ViewClass,
            model
        } );
    }

    create( name ) {
        const component = this._components.get( name );

        const model = component.model;
        const view = new component.ViewClass( model );
        const controller = new component.ControllerClass( this.editor, model, view );

        // Ouch... this will leak the memory. How can we keep the instances to avoid that?
        // (No, weak maps are not the answer here).
        this._instances.add( {
            name,
            view,
            controller,
            model
        } );

        return controller;
    }
}

See the comment inside the create() method.

We talked with @fredck that it would be nice if someone could retrieve all instances of a specific button (e.g. all bold buttons) in order to extend their behaviour in some way.

In order to do that, we need to store instances of those components. However, doing this like above will lead to a memory leak. E.g. the buttons will stay in the memory even though the toolbar containing them was destroyed.

I think though that this could be solved by introducing a controller#destroy event on which the repository would listen and delete a certain component.

However, I'm not going to implement this for now. I don't want to waste time on this as the case is quite edgy anyway.

@Reinmar
Copy link
Member Author

Reinmar commented Feb 19, 2016

We talked with @fredck that it would be nice if someone could retrieve all instances of a specific button (e.g. all bold buttons) in order to extend their behaviour in some way.

To clarify this – I don't think that anyone should be doing this, but well... we can expect the developer will try to extend existing functionality in this way.

@Reinmar
Copy link
Member Author

Reinmar commented Feb 19, 2016

I've pushed initial implementation (see ckeditor/ckeditor5-core#209 (comment)). I think that there's nothing more I can implement now, because things like feature localization isn't defined, neither are features (see #129). I'm going to write some docs and tests and push it on review.

@Reinmar
Copy link
Member Author

Reinmar commented Mar 1, 2016

I've discussed with some of you the shape of the first implementation and we came up with some changes. Some things in the initial implementation didn't have much sense (mainly - code placement), but I think that it's now more clear what should land where.

I've put some proposal in https://github.com/ckeditor/ckeditor5-design/wiki/UI-Library. At the end it contains some ideas that may be worth considering. I'd like to make a decision about https://github.com/ckeditor/ckeditor5-design/wiki/UI-Library#directory-structures asap, because I'll be making other changes in the library and I can do this as well.

@Reinmar Reinmar added the review? label Mar 1, 2016
@oleq
Copy link
Member

oleq commented Mar 1, 2016

https://github.com/ckeditor/ckeditor5-design/wiki/UI-Library#directory-structures asap, because I'll be making other changes in the library and I can do this as well.

I know that

src/buttonview.js
src/buttonmodel.js
src/toolbarview.js

is simpler, but the architecture must allow

src/buttonview.js
src/buttonmodel.js
src/hugecomponent/view.js
src/hugecomponent/...and zillion files more

in case some component has extra dependencies, static libraries etc. Otherwise, a single exception to the simple structure will end up with a mess because children of src/hugecomponent/ mustn't necessarily start with ^hugecomponent\w+\.js. And given that there are dozens of files under src/ it will look like:

src/buttonview.js
src/buttonmodel.js
src/hugecomponentview.js
...many, many files here
src/some-library-that-hugecomponent-depends-on.js // how to decouple it from other files?

So to KISS I'd simply stay with src/component/*.js structure.

@fredck
Copy link
Contributor

fredck commented Mar 1, 2016

@oleq:

So to KISS I'd simply stay with src/component/*.js structure.

+1

@fredck
Copy link
Contributor

fredck commented Mar 1, 2016

When it comes to 3rd Party UI Components (btw, it should be "Third-party"), we may do it by convention (fixed directory name, like ui/mycomponent/* or configuration in package.json). The overriding order stays as a problem though:

  1. The Base-Library, an optional package.json configuration for The Library.
  2. The Library.
  3. The ui directories in the packages, no order guaranteed :/

@fredck
Copy link
Contributor

fredck commented Mar 1, 2016

As for the rest, it looks great for me.

@Reinmar
Copy link
Member Author

Reinmar commented Mar 1, 2016

Thanks!

The third-party UI components worries me though, because that before-great architecture becomes a small mess after considering them :D.

I think that a requirement that we could make is that additional UI components, which are meant to become part of the UI lib, should be defined in a separate ckeditor5-ui-* package. Allowing them inside ui/ directory of a normal package would be more confusing I think. While building the editor, paths would change drastically and there would be a problem that the components don't work in their original directories where some components that would base on them would look for them.

Another thing is that exposing UI components in packages has a bit higher chance of avoiding name collisions because developers will have an easier job of controlling what UI components are getting into the build.

Having this requirement, the builder could do the following:

  1. Copy ckeditor5-ui-default (always).
  2. Copy other ckeditor5-ui-* packages that are installed (because we consider them dependencies of the loaded features) but aren't specified in the builder's configuration as "UI library extensions and replacements".
  3. Copy ckeditor5-ui-* packages that were specified in builder's configuration as "UI library extensions replacements".

I think that it will create the correct structure. And the developer will have an option to fine-tune the order by the builder configuration (moving more packages from 2nd to 3rd step). But it's extremely unlikely that anyone will ever need to do this, because, let's be honest – extensions for third-party UI components will not happen that often :D.

@Reinmar
Copy link
Member Author

Reinmar commented Mar 1, 2016

BTW. That building steps may look easy to implement, but there'll be a lot of work to do so. Especially the watcher's implementation will require an algorithm for resolving whether a file coming from some package would be the last version copied to ui/ or not :D.

@fredck
Copy link
Contributor

fredck commented Mar 1, 2016

+1 for #133 (comment)

@Reinmar Reinmar removed the review? label Aug 17, 2016
@Reinmar
Copy link
Member Author

Reinmar commented Apr 20, 2018

Cleaning up old discussions. See #186.

@Reinmar Reinmar closed this as completed Apr 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants