Plugin requires as strings

[pjasiun]

Sometimes we want to add a plugin to the editor which is already built. In such a case, we can not import requirements. We expect that they are already loaded.

However since this.editor.plugins.get( 'Plugin' ); will return undefined if the Plugin is not loaded, there will be no human-friendly error what is going on.

This is why I think we should introduce strings in requires() function:

	static get requires() {
		return [ 'Plugin' ];
	}

Should enable Plugin if it is avaialbe or show a human-friendly error, if not.

[jodator]

I think that also most (all) of plugins should define pluginName() - mostly sub plugins of a feature like FeatureUI or FeatureEditing.

AFAIR there was a ticket for that in main repo.

[jodator]

Does this issue could solve this: https://github.com/ckeditor/ckeditor5-image/issues/255?

From @Reinmar's insights on why not including Clipboard:

Yes, it would fail. But we do the same thing with other feature-feature relations. This simplifies recomposing the editor with e.g. a bit different implementation of one of those features.

would IMO allow to provide different implementation of Clipboard or Notification. Also the part about human-friendly error would be very nice.

[pjasiun]

I think that also most (all) of plugins should define pluginName() - mostly sub plugins of a feature like FeatureUI or FeatureEditing.

pluginName does not help here. If the plugin is not loaded it cannot be loaded based on the name. So if we require a plugin by name (string) there are 2 options:

• it is already loaded - then we do nothing,
• it is not loaded - then all we can do is to log the error.

[jodator]

it is not loaded - then all we can do is to log the error.

Forget about the pluginName. String in requries() would improve DX greatly IMO. Since error like "The required plugin 'Clipboard' is not available" is far better then:

TypeError: Cannot read property 'Symbol(emitterId)' of undefined
    at _getEmitterId (webpack:///packages/ckeditor5-utils/src/emittermixin.js:453 <- build/.automated-tests/entry-point.js:85578:16)
    at ImageUploadEditing.listenTo (webpack:///packages/ckeditor5-utils/src/emittermixin.js:80 <- build/.automated-tests/entry-point.js:85205:9)
    at ImageUploadEditing.init (webpack:///packages/ckeditor5-image/src/imageupload/imageuploadediting.js:101 <- build/.automated-tests/entry-point.js:76259:8)

I'd see this as "soft" dependency as opposite to "hard" (class taken from import).

[Reinmar]

Confirmed and added to the next iteration. Since we rely right now in some places on these "loosen" dependencies, let's have better error reporting for them.

[ma2ciek]

Maybe I'm wrong here, but the require() can inject missing plugins and initialize them during the bootstrap.

Taking that code:

static get requires() {
	return [ 'Plugin' ];
}

You'd have to load the Plugin manually as it can't be resolved.

However since this.editor.plugins.get( 'Plugin' ); will return undefined

Maybe this is the place that can be changed to achieve a similar effect? We can provide a method that may throw if the plugin isn't defined yet.

[Reinmar]

I think that it's good if it fails early, and requires() will fail earlier than when get() is used.

[Reinmar]

One thing to remember is that plugins from requires() need to be initialised before the plugin that depends on them. This should also be true for string requirements.

[Reinmar]

BTW, this will not be that trivial to implement. The plugin which is required by its name may be a 3rd dependency in a chain of some other plugin deps, so basically, before loading anything, we need to scan the entire tree of dependencies and store those plugins in a map. I'm not sure whether we're doing this already so this feature may require a bit more refactoring.

[jodator]

This is why I think we should introduce strings in requires() function:

I'm not sure whether we're doing this already

To my surprise this has limited support right now. What we lack is the human readable error - right now generic load error is thrown.

What I think we lack right now is:

• clear statement which plugins should by loaded by Class and which by String (document somewhere?)
• human-friendly error for situations where one plugin could not be loaded by name in it's requires().
• human-friendly error when suing plugins.get() rather then returning undefined

Do we need harder checking of dependencies? Like building dependency tree before instantiating editor? One plus would be such that before instantiating plugins we could load all required and then check if all are available so one could not worry about order of defining dependencies (by string) as such might be loaded by Class by other plugin later.

The plugin which is required by its name may be a 3rd dependency in a chain of some other plugin deps, so basically, before loading anything, we need to scan the entire tree of dependencies and store those plugins in a map.

For starters I'd go with human-friendly errors and I'd refactor features "soft" dependencies so they would be in the requires() block.

The I'd go with loading plugins in two-step process (as describe earlier):

1. scan dependencies and load them into map
2. try to load editor in proper dependency order (as this is done right now)

ps.: I might rethink it so I don't propose it as final

[jodator]

Below is current state of dependencies:

• internal - requries() to the same package (E/UI - editing / UI parts of feature).
• external - requires() to other package.
• .get() - any editor.plugins.get() to a package that is not in requires().

Plugin	internal	external	.get()
UploadAdapter		FileRepository
Alignment	E/UI
AutoSave		PendingAction
Bold	E/UI
Code	E/UI
Italic	E/UI
Strikethrough	E/UI
Subscript	E/UI
Superscript	E/UI
Underline	E/UI
BlockQuote	E/UI
CKFinder	E/UI	CKFinderUploadAdapter	Notification
BalloonEditorUI			'BalloonToolbar'
CKFinderUploadAdapter		FileRepository
CKFinderEditing		Notification
CSUploadAdapter		FileRepository CloudServices
EasyImage		CSUploadAdapter Image ImageUpload
Essentials		Clipboard Enter ShiftEnter Typing Undo
Font	FontFamily FontSize
FontFamily	E/UI
FontSize	E/UI
Heading	E/UI
HeadingEditing		Paragraph
Highlight	E/UI
Image	E/UI ImageTextAlternative	Widget
ImageCaption
ImageStyle	E/UI
ImageTextAlternative	E/UI
ImageToolbar		WidgetToolbarRepo
ImageUpload	E/UI ImageUploadProgress
ImageStyleEditing	ImageEditing
ImageTextAlternative		ContextualBalloon
ImageUploadEditing		FileRepository Notification	'Clipboard'
Link	E/UI
LinkUI		ContextualBalloon
List	E/UI
ListEditing		Paragraph
AutoMediaEmbed		Clipboard Undo
MediaEmbed	E/UI AutoMediaEmbed	Widget	Clipboard
MediaEmbedToolbar		WidgetToolbarRepo
MediaEmbedUI	MediaEmbedEditing
PFO			Clipboard
Table	E/UI	Widget
TableEditing	TableUtils
TableToolbar		WidgetToolbarRepo
Typing	Input Delete
BalloonToolbar		ContextualToolbar
Undo	E/UI
FileRepository		PendingActions
WidgetToolbarRepo		ContextualToolbar	'BalloonToolbar'

From above I can see some inconsistencies or possible bugs:

• MediaEmbedUI requires MediaEmbedEditing (bug?)
• ImageStyleEditing requires ImageEditing (bug?)
• Unify 'TableUtils' calls across feature (they're either string or class)
• AutoMediaEmbed has dependency on Clipboard (should be string) and on Undo (could be as a string - it uses 'undo' command)

As for refactoring:

1. I'd leave all internal requires() as imports
2. The Essentials plugins pre-loads essential dependencies and it must have class imports
3. The Clipboard plugins should be always get by 'Clipboard' and thus should be string in requires()
4. EasyImage might use Image, ImageUpload as strings. The CSUploadAdapter should be required as class.
5. Plugins that might be required as strings but must be added to builds (or to the Essentials or something new maybe?):
   • Notification
   • FileRepository
   • Widget
   • WidgetToolbarRepository
   • PendingAction
   • Paragraph (would allow others to re-implement Paragraph feature)
   • ContextualToolbar
   • BalllonToolbar

---

Sorry for lengthy post :) @pjasiun & @oleq what do you think on the above? Also what should be the rule for requires? This look logical for me. Also I'd update this guide with what we agree on. (also the feature split part is obsolete now AFAIR).

[pjasiun]

First of all, thanks for the great research!

Second: my original post was not about making any big changes in the way the current dependencies works now, more about introducing new feature which might be useful in some cases.

This is why I think that:

1. dependencies which are loaded using .get() (not mentioned in requires) and are not needed for the plugin to work, should be still loaded using .get().
2. dependecies which are loaded using .get() (not mentioned in requires) and are needed for the plugin to work, should be added to the requires (as a class).
3. Widget, FileRepository, etc. should be still loaded as they are loaded now (as class dependencies). At the same time, I think that the idea of changing it to the string dependency is very cool, but we should consider it in the follow-up (separate ticket) since it is a much bigger change.
4. I do not think anything needs to be changed to the string dependencies at this point, but I might not know about something. This feature, at this point, is more useful for samples where we sometimes add plugins to builds without rebuilding them.

However, when it comes to the idea of having Widget, FileRepository, etc. as string dependecis - I see pros and cons of such change. On the one hand, I like the fact that so far these plugins are just utils for other plugins, that the end-user does not need to know anything about them. End-users can focus on loading only end-users features which loades all they need authomatically. On the other hand, this is one of the reasones one can not add widgets to buids. But I am not sure if changing these dependecies will be enough. I would have it only if we will be sure that this is a way to solve dynamic plugins loading issue.

[jodator]

• dependencies which are loaded using .get() (not mentioned in requires) and are not needed for the plugin to work, should be still loaded using .get().

• dependecies which are loaded using .get() (not mentioned in requires) and are needed for the plugin to work, should be added to the requires (as a class).

This boils down to how to deal with situations like this for which requires( [ 'Clipboard' ] ) will help and is not a big deal to process AFAICS. Bus sole warning in the get( 'Clipboard' ) will also help.

Also such constructs:
https://github.com/ckeditor/ckeditor5-image/blob/cd2a8fd335708922efd400ad541bf09cd3d28d50/src/imageupload/imageuploadediting.js#L101

Should it be enclosed in if( clipboardPlugin ){} as the plugin might be not loaded (I know right now the tests, but anyway...)?

I would review current state and add such checks for plugins that are not needed.

Also this ticket is about adding string requires (look your first post ;) ). The editor.plugin.get( 'string' ) right now works but lacks human-readeable warning which will be added.

Adding those not needed plugins to the requires() would improve DX.

Anyway I would add them as "string" dependency to requires() - the waning will be logged earlier and better for the dev. Thus I'm not pushing the bigger refactor but only adding string requires as proposed in the begging:

Should enable Plugin if it is avaialbe or show a human-friendly error, if not.

[pjasiun]

We talked F2F with @jodator and agreed that it will be better if editor.plugins.get will throw an exception if the plugin is not loaded and we introduce editor.plugins.has for cases where some dependencies are optional. This way, it will be more explicit and more consistent, it will not be possible to get some dependency which is not required and get misleading error.

[jodator]

ps.: The below construct is valid in current code:

static get requires() {
	return [ 'Plugin' ];
}

[Reinmar]

ps.: The below construct is valid in current code:

It's actually not. You cannot require a plugin by a string. It must be a function.

[Reinmar]

Finally, I proposed to get rid of the concept of soft requirements as we don't use it frequently enough to make a difference: https://github.com/ckeditor/ckeditor5-core/issues/193.

[jodator]

It's actually not. You cannot require a plugin by a string. It must be a function.

But you can:

https://github.com/ckeditor/ckeditor5-core/blob/5b2507b5494fa26a94d970057fa69aeb9426dbeb/src/plugincollection.js#L258-L278

It will work as long as the required plugin is loaded.