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

Case for CKEDITOR.config #178

Closed
Reinmar opened this issue May 24, 2016 · 5 comments
Closed

Case for CKEDITOR.config #178

Reinmar opened this issue May 24, 2016 · 5 comments
Labels
status:discussion type:feature This issue reports a feature request (an idea for a new functionality or a missing option). type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@Reinmar
Copy link
Member

Reinmar commented May 24, 2016

Right after we removed that feature we found a pretty interesting use case for a global config :D.

The case is bundling with Rollup. This requires a file which implements an entry point for such a bundle. It must import all necessary files, including features and creators (which are not referenced anywhere else). Only then Rollup will include them.

The basic implementation can therefore look like this:

import CKEDITOR from './ckeditor.js';
import FeatureA from './feature-a/a.js';
import CreatorB from './creator-b/b.js';

const defaultConfig = () => {
    return {
        creator: CreatorB,
        features: [ FeatureA, ... ]
    };
};

export default createEditor( elements, config ) {
    return CKEDITOR.create( elements, Object.assign( defaultConfig(), config ) );
}

However, it would be great if it could simply export the CKEDITOR object. If we had a global config we could do:

import CKEDITOR from './ckeditor.js';
import FeatureA from './feature-a/a.js';
import CreatorB from './creator-b/b.js';

CKEDITOR.config.set( {
    creator: CreatorB,
    features: [ FeatureA, ... ]
} );

export default CKEDITOR;

Much, much nicer.

@Reinmar Reinmar added type:improvement This issue reports a possible enhancement of an existing feature. type:feature This issue reports a feature request (an idea for a new functionality or a missing option). status:discussion labels May 24, 2016
@Reinmar
Copy link
Member Author

Reinmar commented May 24, 2016

There's another topic, which is related to this one and will need to be addressed: ckeditor/ckeditor5-design#140

In short – editor will need to be subclassable and the idea of creators didn't work. Creators have no sense of existence, because they try to inject some logic into editors, by extending them. And the cleanest way to do this is by inheritance, not by a separate class which is initialised too late to do serious stuff and should not override some existing implementations, what greatly limits it.

So, if we'll go with subclassable editors, and we simply don't see any other way at the moment, we'll be able to solve the issue with Rollup in a totally different way:

import StandardEditor from './editor/standardeditor.js';
import FeatureA from './feature-a/a.js';

export default class BuildStandardEditor extends StandardEditor {
    constructor( ... args ) {
        super( ...args );

        this.config.features.push( FeatureA );
    }
}

Alternatively, we could keep the same logic on static Editor.create method which could be quite useful (note: I think that elements should be moved to config):

class Editor {
    ...

    // @returns {Promise}
    static create( config ) {
        return new this( config ).init();
    }
}

// For Rollup:

import StandardEditor from './editor/standardeditor.js';
import FeatureA from './feature-a/a.js';

export default class BuildStandardEditor extends StandardEditor {
    static create( config ) {
        // That's modifying the config object, so cloning would be necessary perhaps.
        config.plugins = [ ...config.plugins, FeatureA ];

        return super( config );
    }
}

@Reinmar
Copy link
Member Author

Reinmar commented May 24, 2016

In case it wasn't clear – my previous comment points out that the global config isn't necessary if we change the architecture.

@fredck
Copy link
Contributor

fredck commented May 24, 2016

global config isn't necessary if we change the architecture

What is the big deal with it anyway? Is it worth this whole refactoring effort for something which was quite simple?

@Reinmar
Copy link
Member Author

Reinmar commented May 24, 2016

We need to change the architecture anyway. So if we can avoid adding a code that we don't explicitly need, then we should do that. And currently there's very little value that a global config could give us. If a clear use cases for it will appear, then we certainly implement it.

@Reinmar
Copy link
Member Author

Reinmar commented Jan 25, 2018

We implemented the static EditorClass.build property for this purpose: https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/builds/guides/development/installing-plugins.html#difference-between-both-methods. Turned out to be a bit more than just a global config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:discussion type:feature This issue reports a feature request (an idea for a new functionality or a missing option). type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

2 participants