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

Plugins loading improvements #5602

Closed
jodator opened this issue Oct 16, 2019 · 5 comments
Closed

Plugins loading improvements #5602

jodator opened this issue Oct 16, 2019 · 5 comments
Labels
resolution:expired This issue was closed due to lack of feedback. status:stale type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@jodator
Copy link
Contributor

jodator commented Oct 16, 2019

📝 Provide a description of the new feature

The plugin require/exclude chain fails in certain scenarios ([1]).

Problem description

Example requires() chain:

Foo         -> FooEdtinng, FooUI, Paragraph
FooBaz      -> (no deps but extends FooEditing - like schema items - 
                and is usualy used with Foo anyway)
Bar         -> BarEditing, BarUI
BarEditing  -> FooEditing

Scenario 1:

plugins = Paragraph, Foo, Bar
removePlugins = Foo

plugins that will be loaded (in that order):

Paragraph, FooEditing, BarEditing, BarUI, Bar

This might be "counter intuitive" since FooEditing will work eventhough Foo was removed.

Scenario 2:

plugins = Paragraph, Foo, FooBaz
removePlugins = Foo

The plugin FooBaz will fail because it uses FooEditing

Scenario 2 might be fixed by adding FooEditing to the requires() call.

The main reason for this is that in such scenario that removePlugins forbids loading this and only this plugin.

The real-life examples of plugins causing problems: Image (Foo), ImageEditing (FooEditing), ImageUploadEditing (FooBaz), Bar (CKFinder), BarEditing (CKFinderEditing).

Solution

I think that we could fix that with somthing similar to "softRequirement". The idea is to differantatie between plugins loaded together (thus if main plugin is removed then it's children also) and plugins that are required by this plugin to work.

Plugin.depends() - tl;dr: current Plugin.requires() - will load a plugin before and will yield errors if plugin is not loaded (ie by removePlugins)
Plugin.bundles() - loads those plugins with current one if current plugins is removed then all bundled are removed also.

In the plugin chain from above:

Foo         -> bundles: FooEdtinng, FooUI,
            -> depends: Paragraph

FooBaz      -> depends: FooEditing

Bar         -> bundles: BarEditing, BarUI

BarEditing  -> depends: FooEditing

This way in both scenarios the editor will throw error because required plguin FooEditing of the BarEditing plugin was removed using removedPlugins call.

Real world examples:

  1. Bold:
    • depends: -
    • bundles: BoldEditing, BoldUI
  2. Table:
    • depends: Widget
    • bundles: TableEditing, TableUI
  3. EasyImage:
    • depends Image, ImageUpload (probably should be Editing parts),
    • bundles CloudServicesUploadAdapter

Cons:

  • another API breaking change :(

If you'd like to see this feature implemented, add a 👍 reaction to this post.

@jodator jodator added the type:feature This issue reports a feature request (an idea for a new functionality or a missing option). label Oct 16, 2019
@jodator
Copy link
Contributor Author

jodator commented Oct 16, 2019

ps.: We should also check what plugins should depends on other like ImageUploadEditing does not requires() the ImageEditing but it depends on image being defined in the schema.

@Reinmar
Copy link
Member

Reinmar commented Oct 16, 2019

My biggest surprise here is that I never got https://github.com/ckeditor/ckeditor5-core/blob/ee4f9dbb2974c5461efb75b751448ad962104ac5/src/plugincollection.js#L261-L275 when testing. And I should at some point. So, I think that something is seriously broken there.

I knew this situation may occur when implementing plugin collection so I specifically wrote this error for it.

I guess there are more potential problems with removing plugins, so we can investigate them. But our goal doesn't need to be to make the editor somehow work with some deep dependencies removed. It's ok if it just throws. The developer will be forced to review what they want to remove from the editor and why it cannot be removed or what else needs to be removed with it.

@Reinmar Reinmar added this to the next milestone Oct 16, 2019
@Reinmar
Copy link
Member

Reinmar commented Oct 16, 2019

Adding to the next milestone because this is a pretty serious problem. config.removePlugins is one of the most basic options and it should just work. But my experience is that it doesn't.

@pomek pomek modified the milestones: next, nice-to-have Nov 10, 2020
@pomek pomek removed this from the nice-to-have milestone Feb 21, 2022
@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).

@CKEditorBot CKEditorBot added the resolution:expired This issue was closed due to lack of feedback. label Nov 5, 2023
@CKEditorBot CKEditorBot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolution:expired This issue was closed due to lack of feedback. status:stale type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

No branches or pull requests

4 participants