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

Plugin Configuration: Support Optional Dependencies to Plugins #12199

Open
mmichaelis opened this issue Aug 2, 2022 · 5 comments
Open

Plugin Configuration: Support Optional Dependencies to Plugins #12199

mmichaelis opened this issue Aug 2, 2022 · 5 comments
Labels
package:core squad:core Issue to be handled by the Core team. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@mmichaelis
Copy link

mmichaelis commented Aug 2, 2022

📝 Provide a description of the new feature

We have a challenge similar to the GHS feature: Integrate to other plugins, if they are installed.

GHS does so by listing to the event init of editor.data:

/**
* Registers elements allowed by {@link module:html-support/datafilter~DataFilter#allowElement} method
* once {@link module:engine/controller/datacontroller~DataController editor's data controller} is initialized.
*
* @private
*/
_registerElementsAfterInit() {
this.editor.data.on( 'init', () => {
this._dataInitialized = true;
for ( const definition of this._allowedElements ) {
this._fireRegisterEvent( definition );
}
}, {
// With highest priority listener we are able to register elements right before
// running data conversion. Also:
// * Make sure that priority is higher than the one used by `RealTimeCollaborationClient`,
// as RTC is stopping event propagation.
// * Make sure no other features hook into this event before GHS because otherwise the
// downcast conversion (for these features) could run before GHS registered its converters
// (https://github.com/ckeditor/ckeditor5/issues/11356).
priority: priorities.get( 'highest' ) + 1
} );
}

As you can see, it contains a lot of inline comments, a comment about "how to carefully select a priority", which makes us feel: Doing it the same way may easily break, unless CKEditor provides a contract, that this will work.

Use Case Details

As sometimes, it is easier understanding the use-case (to think of possible alternatives), here is ours:

  • We have some server-side differencing approach.
  • This adds some new elements, but it also adds additional attributes to images, for example.
  • Similar to GHS, we want to forward these additional attributes from data over model to editing view for images.

In our solution, we want to "optionally depend" not only on the Image-Plugin (inlineImage), but also on GHS' htmlImg, if the Image-Plugin is not installed.

Suggestion

Without knowing details, something like this may provide a more general solution:

static get requires() {
  return [ SomePlugin ];
}

static get optionallyDependingOn() {
  return [ ImageInline, ImageElementSupport ];
}

init() {
  const { editor } = this;
  const { model, plugins } = editor;
  const { schema } = model;

  if (!plugins.has("ImageInlineEditing")) {
    return;
  }

  // Ensured, that if `ImageInlineEditing` is available, its init() method got called prior to this.
  if (schema.isRegistered("imageInline")) {
    schema.extend("imageInline", {
      allowAttributes: ["myAttribute"],
    });
  }
}

optionallyDependingOn should adjust the initialization order, in that way, that, if ImageInline plugin is available, that the "optionally depending" plugin's init() method is called after the init() method of ImageInline.


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

@mmichaelis mmichaelis added the type:feature This issue reports a feature request (an idea for a new functionality or a missing option). label Aug 2, 2022
mmichaelis added a commit to CoreMedia/ckeditor-plugins that referenced this issue Aug 2, 2022
For integrating into optionally available plugins, we need to wait
for their schema elements to be registered, prior to extending
these registrations.

Added a similar approach as for GHS, to workaround any other
better approach, as suggested in ckeditor/ckeditor5#12199.
@Reinmar Reinmar added squad:core Issue to be handled by the Core team. package:core labels Aug 3, 2022
@scofalik
Copy link
Contributor

scofalik commented Aug 3, 2022

Interesting subject.

If you need to, for now, you can workaround this by using afterInit() method of Plugin. afterInit()s are called after all init() calls are done. So, at this point, you can make your plugins.has() check and be sure that the plugin was initialized.

When it comes to what DataFilter class is doing, I wonder if there was any problem putting this code in afterInit(). Unfortunately, the comment does not mention that, so I believe that it simply could have been missed.

What I see in DataFilter looks more like a hack, as you should not introduce feature logic in a callback that is responsible for data handling. Maybe what we need is an additional event in the editor initialization process, where we know that all plugins are initialized but we want to do some processing/set-up before starting working on the document data?

@mmichaelis
Copy link
Author

Hi @scofalik,

When it comes to what DataFilter class is doing, I wonder if there was any problem putting this code in afterInit(). Unfortunately, the comment does not mention that, so I believe that it simply could have been missed.

I did not try it, but one of my colleagues, and he reported, it failed – at least trying to hook into the htmlImg from GHS. For inlineImage it even worked directly with schema.isRegistered("imageInline") but reading the code of DataFilter this seems to be pure luck. Is there even a granted "initialization order" of plugins? Like the order plugins are mentioned in configuration? But such an approach would be error-prone.

What I see in DataFilter looks more like a hack, as you should not introduce feature logic in a callback that is responsible for data handling. Maybe what we need is an additional event in the editor initialization process, where we know that all plugins are initialized but we want to do some processing/set-up before starting working on the document data?

Yes, it looks like a hack, but reading it, it looks as if it got developed having to deal with several pains in initialization order.

For the given use-case, such an extra event would help. But plugins should not register more elements at this stage, they should only be allowed to extend existing registered elements. Otherwise, if plugin A registers a new element in this new event, plugin B could not extend it or struggles with the same issues.

Side note: For the htmlImg use-case we found a different solution meanwhile, which is the advanced checkAttribute as considered in your documentation:

/*
 * As GHS registers `htmlImg` late in CKEditor's initialization process,
 * there does not seem to be a good/robust way to extend `htmlImg` element.
 * That is why we instead listen to `checkAttribute` to handle this case
 * on demand.
 */
schema.on(
  "checkAttribute",
  (evt, args) => {
    const context = args[0];
    const attributeName = args[1];

    if (context.endsWith("htmlImg") && attributeName === "xdiff-changetype") {
      // Prevent next listeners from being called.
      evt.stop();
      // Set the checkAttribute()'s return value.
      evt.return = true;
    }
  },
  { priority: "high" }
);

This works like a charm. Kudos to your documentation.

@scofalik
Copy link
Contributor

scofalik commented Aug 3, 2022

For the given use-case, such an extra event would help. But plugins should not register more elements at this stage, they should only be allowed to extend existing registered elements. Otherwise, if plugin A registers a new element in this new event, plugin B could not extend it or struggles with the same issues.

Yes, of course, we should have some simple and clear rules what is a good place to do what. We miss that right now, so the rules are basically:

  • everything should be done in init() if possible,
  • everything else has to be done in afterInit().

@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.

@rgpublic
Copy link

rgpublic commented Oct 2, 2023

Yes, I'd still be interested in a better dependency management. Now that the CKE5 plugin ecosystem grows and thrives and more plugins will come out that depend on one another, there's growing demand I guess to define precise dependencies somewhat along the lines of what's been proposed in the issue summary. That is, a plugin should be able to specifically say "I depend on other plugin XY and want to be initialized after it". Otherwise we'd have to resort to afterInit(), afterAfterInit(), afterInitButBeforeAfterAfterInit() and so on. It'll be a mess. So I'll definitely keep up my +1 to this. Thank you :-)

PS, sorry I forgot... See this e.g. for a current issue where this has become a problem: https://www.drupal.org/project/ckeditor_media_resize/issues/3387079

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:core squad:core Issue to be handled by the Core team. 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

5 participants