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

Affect plugins loading order without creating strict requirements #2907

Closed
pjasiun opened this issue Aug 27, 2019 · 9 comments
Closed

Affect plugins loading order without creating strict requirements #2907

pjasiun opened this issue Aug 27, 2019 · 9 comments
Assignees
Labels
package:core status:discussion type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@pjasiun
Copy link

pjasiun commented Aug 27, 2019

We already touched this topic here, but it was not implemented then.

Currently, when one plugin requires another plugin you can be sure about two things:

  • the required plugin will be loaded,
  • the required plugin will be loaded before your plugin.

However, sometimes we need only the second part. Another plugin is not necessary, but if it is loaded we want it to be initialized before our plugin. Currently afterInit needs to be used what is far from a perfect solution. My proposal is to introduce soft requirements:

class A extends Plugin {
	static get softRequires() {
		return [ 'B' ];
	}

	static get pluginName() {
		return 'A';
	}

	init() {
		if( this.editor.plugins.has( 'B' ) {
			console.log( 'B is also loaded!' );
		}
	}
}

class B extends Plugin {
	static get pluginName() {
		return 'B';
	}
}

It means that if the plugin B is not loaded then plugin A shows no errors nor warnings. However, if plugin B is loaded, then it is initialized before plugin A, the same way, as it would be a requirement.

Note, that I am not sure about "Soft requirements" name nor about softRequires method name. Feel free to propose a better name.

@Reinmar
Copy link
Member

Reinmar commented Aug 27, 2019

Yeah, I think I discussed this recently with someone. A different option appeared, AFAIR, but in general, the same concept. So 👍 for doing this.

I think the other option could be to have all in the requires() section but some plugins would be defined by name (string == soft) and some by class (function == hard). But I think that a more descriptive softRequires() is actually better.

@Reinmar
Copy link
Member

Reinmar commented Aug 27, 2019

OK, @pjasiun reminded me that in https://github.com/ckeditor/ckeditor5-core/issues/148 we already added "by name" support in requires(). I completely forgot about that 😃

So, I'm actually hesitating now. If the order of plugins is the only problem here, then until we really see a need for something more I'd use afterInit(). So far it worked well – all code which registers something goes to init() and all which uses the information about registered "things" to afterInit().

Of course, this mechanism must not be abused because you can create chains of "do X if Y", "do Y if Z", etc., but this is usually easy – you can postpone making decisions how your feature should behave to later stages (e.g. command's refresh logic) and then you completely untangle two features.

Also, a completely extreme version of a chain is a loop of requirements and even soft requires would not be a solution here. You need to make reasonable decisions when implementing relations between features.

Finally, I think that the "soft requirement" mechanism has a very narrow application – only to cases where a dependent plugin knows precisely with which other plugins it should work. In most cases this isn't true – e.g. Autoformat should not assume that it needs Bold because the bold functionality could be provided by 3rdPartyBold. Actually, people usually prefix their extensions, so this is a real case.

So, let's wait for a clear signal that we miss something.

@Reinmar
Copy link
Member

Reinmar commented Aug 27, 2019

Plot twist: I've been analysing https://github.com/ckeditor/ckeditor5-core/issues/148 and I was right by thinking we don't support string requirements. This ticket didn't actually introduce them :D

I'll create a new ticket for that concept.

@Reinmar Reinmar changed the title Soft requirements Affecting plugins loading order without creating strict requirements Aug 27, 2019
@Reinmar
Copy link
Member

Reinmar commented Aug 27, 2019

Another plot twist: I proposed to get rid of soft requirements (in the sense we used them in the past) completely to acknowledge the current state of things: https://github.com/ckeditor/ckeditor5-core/issues/193.

@Reinmar Reinmar changed the title Affecting plugins loading order without creating strict requirements Affect plugins loading order without creating strict requirements Aug 27, 2019
@jodator
Copy link
Contributor

jodator commented Aug 29, 2019

Read like a good novel with so much plot twists :D

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-core Oct 9, 2019
@mlewand mlewand added this to the backlog milestone Oct 9, 2019
@mlewand mlewand added status:discussion type:improvement This issue reports a possible enhancement of an existing feature. package:core labels Oct 9, 2019
@jodator
Copy link
Contributor

jodator commented Dec 3, 2020

Ha! Another plot twist. We will probably have to loosen some requires() calls due to work on CKEditor5 DLL (#8395). What we will need here:

  • Probably new method alongside requries() that will define softRequires() - I'm unsure about the naming but let leave it as is for now.
  • The softRequires() should be used to cross-package requirements definition.
  • The requires() should be left as now for "glue" packages as an automatic plugin loading mechanism for plugins from the same package. In most cases, it would be that Feature will require FeatureEditing and FeatureUI.

@jodator jodator self-assigned this Dec 9, 2020
@jodator
Copy link
Contributor

jodator commented Dec 9, 2020

Again, the same finding as before IIRC:

  • The Plugin.requires() can have string. It will try to load a plugin from builtinPlugins (the availablePlugins in PluginCollection.
  • I've modified a bit plugin loading to also allow loading plugins from plugins object (so re-ordering them).
  • If a plugin is unavailable (either builtinPlugins or plugins array) we can reject editor loading (right now it doesn fail).

Last changes: https://github.com/ckeditor/ckeditor5/compare/dll-integration...i/2907?expand=1

@jodator
Copy link
Contributor

jodator commented Dec 10, 2020

After a F2F talk, we agreed that there's no need to introduce new softRequires getter as we already allow a string to be specified there. The current behavior will be expanded to order plugins available either as Editor.builtinPlugins or plugins / extraPlugins configuration option. ATM, the string only loads a plugin if it is defined in builtinPlugins.

pomek added a commit that referenced this issue Dec 11, 2020
Internal: Plugin collection will allow to require plugin by name when it is provided in `config.plugins` or if it was already loaded. Closes #2907.
@pomek
Copy link
Member

pomek commented Dec 11, 2020

Closed with 9c24fca.

@pomek pomek closed this as completed Dec 11, 2020
@Reinmar Reinmar modified the milestones: backlog, iteration 39 Dec 14, 2020
Reinmar added a commit that referenced this issue Feb 1, 2021
Other: Enabled creating builds that can be extended (with more plugins) without the need to being recompiled. This required splitting the project into the, so-called, DLL part and consumers of this DLL. Under the hood, the mechanism is based on [webpack DLLs](https://webpack.js.org/plugins/dll-plugin/). This is the first part of the required changes and it contains the necessary breaking changes (see the "MAJOR BREAKING CHANGES" section above). For more information see the ["DLL builds"](https://ckeditor.com/docs/ckeditor5/latest/builds/guides/development/dll-builds.html) guide. Closes [#8395](#8395).

Other: Introduced `Image.isImageWidget()` utility method.

Feature (core): Plugin collection will allow to require plugin by name when it is provided in `config.plugins` or if it was already loaded. Closes [#2907](#2907).

MAJOR BREAKING CHANGE: Several plugins are not anymore loaded automatically as dependencies of other plugins. From now on, they need to be provided to the editor creator manually (via `config.plugins`). This list includes:

- The `CloudServicesUploadAdapter` plugin no longer loads `CloudServices`. Make sure to add `CloudServices` to the editor plugins when using the  `CloudServicesUploadAdapter` or `EasyImage` features.
- The `EasyImage` plugin no longer loads `Image` and `ImageUpload`. Make sure to add `Image` and `ImageUpload` to the editor plugins when using the `EasyImage` feature.
- The `CKFinder` plugin no longer loads `CKFinderUploadAdapter`. The `CKFinderEditing` plugin no longer loads `ImageEditing` and `LinkEditing` features. Make sure to add `CKFinderUploadAdapter`, `Image`, and `Link` features to the editor plugins when using the `CKFinder` feature.
- The `Title` plugin no longer loads `Paragraph`. Make sure to add `Paragraph` to the editor plugins when using the `Title` feature.
- The `ListEditing` plugin no longer loads `Paragraph`. Make sure to add `Paragraph` to the editor plugins when using the `List` feature.
- The `LinkImageEditing` plugin no longer loads `ImageEditing`. Make sure to add `Image` to the editor plugins when using the `LinkImage` feature.
- The `LinkImageUI` plugin no longer loads `Image`. Make sure to add `Image` to the editor plugins when using the `LinkImage` feature.
- The `ExportPdf` plugin no longer loads `CloudServices`. Make sure to add `CloudServices` to the editor plugins when using the `ExportPdf` feature.
- The `ExportWord` plugin no longer loads `CloudServices`. Make sure to add `CloudServices` to the editor plugins when using the `ExportWord` feature.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:core status:discussion type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

5 participants