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

PluginCollection doesn't keep the order of plugins which were passed to it #2905

Closed
Reinmar opened this issue May 27, 2019 · 3 comments · Fixed by ckeditor/ckeditor5-core#191
Assignees
Labels
package:core type:bug This issue reports a buggy (incorrect) behavior. type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented May 27, 2019

ClassicEditor
	.create( document.querySelector( '#editor' ), {
		plugins: [
			ArticlePluginSet,
			function( editor ) {
				editor.model.schema.extend( 'tableCell', {
					allowAttributes: 'style'
				} );

				editor.conversion.attributeToAttribute( {
					model: {
						name: 'tableCell',
						key: 'style'
					},
					view: 'style'
				} );
			}
		],

I think that this should work because all ArticlePluginSet deps should be initialized before the next plugin from the list is being processed. It fails now because it seems that all the deps are pushed to the end of the queue, so extend() throws that there's no tableCell.

The current behaviour works fine in normal cases, but it's a bit irritating in ones like above.

@jodator jodator self-assigned this Jun 18, 2019
@jodator
Copy link
Contributor

jodator commented Jun 18, 2019

Well the order is OK:

11:52:36.841 plugincollection.js:255 instantiatePlugin ArticlePluginSet
11:52:36.842 plugincollection.js:255 instantiatePlugin Essentials
11:52:36.842 plugincollection.js:255 instantiatePlugin Clipboard
11:52:36.843 plugincollection.js:255 instantiatePlugin Enter
11:52:36.843 plugincollection.js:255 instantiatePlugin ShiftEnter
11:52:36.844 plugincollection.js:255 instantiatePlugin Typing
11:52:36.844 plugincollection.js:255 instantiatePlugin Input
11:52:36.844 plugincollection.js:255 instantiatePlugin Delete
11:52:36.845 plugincollection.js:255 instantiatePlugin Undo
11:52:36.845 plugincollection.js:255 instantiatePlugin UndoEditing
11:52:36.846 plugincollection.js:255 instantiatePlugin UndoUI
11:52:36.846 plugincollection.js:255 instantiatePlugin Autoformat
11:52:36.847 plugincollection.js:255 instantiatePlugin BlockQuote
11:52:36.847 plugincollection.js:255 instantiatePlugin BlockQuoteEditing
11:52:36.847 plugincollection.js:255 instantiatePlugin BlockQuoteUI
11:52:36.848 plugincollection.js:255 instantiatePlugin Bold
11:52:36.848 plugincollection.js:255 instantiatePlugin BoldEditing
11:52:36.852 plugincollection.js:255 instantiatePlugin BoldUI
11:52:36.853 plugincollection.js:255 instantiatePlugin Heading
11:52:36.853 plugincollection.js:255 instantiatePlugin HeadingEditing
11:52:36.853 plugincollection.js:255 instantiatePlugin Paragraph
11:52:36.853 plugincollection.js:255 instantiatePlugin HeadingUI
11:52:36.854 plugincollection.js:255 instantiatePlugin Image
11:52:36.854 plugincollection.js:255 instantiatePlugin ImageEditing
11:52:36.854 plugincollection.js:255 instantiatePlugin Widget
11:52:36.854 plugincollection.js:255 instantiatePlugin ImageTextAlternative
11:52:36.855 plugincollection.js:255 instantiatePlugin ImageTextAlternativeEditing
11:52:36.855 plugincollection.js:255 instantiatePlugin ImageTextAlternativeUI
11:52:36.855 plugincollection.js:255 instantiatePlugin ContextualBalloon
11:52:36.867 plugincollection.js:255 instantiatePlugin ImageCaption
11:52:36.868 plugincollection.js:255 instantiatePlugin ImageCaptionEditing
11:52:36.868 plugincollection.js:255 instantiatePlugin ImageStyle
11:52:36.868 plugincollection.js:255 instantiatePlugin ImageStyleEditing
11:52:36.868 plugincollection.js:255 instantiatePlugin ImageStyleUI
11:52:36.870 plugincollection.js:255 instantiatePlugin ImageToolbar
11:52:36.870 plugincollection.js:255 instantiatePlugin WidgetToolbarRepository
11:52:36.870 plugincollection.js:255 instantiatePlugin Italic
11:52:36.870 plugincollection.js:255 instantiatePlugin ItalicEditing
11:52:36.870 plugincollection.js:255 instantiatePlugin ItalicUI
11:52:36.871 plugincollection.js:255 instantiatePlugin Link
11:52:36.871 plugincollection.js:255 instantiatePlugin LinkEditing
11:52:36.871 plugincollection.js:255 instantiatePlugin LinkUI
11:52:36.871 plugincollection.js:255 instantiatePlugin List
11:52:36.871 plugincollection.js:255 instantiatePlugin ListEditing
11:52:36.872 plugincollection.js:255 instantiatePlugin ListUI
11:52:36.872 plugincollection.js:255 instantiatePlugin MediaEmbed
11:52:36.872 plugincollection.js:255 instantiatePlugin MediaEmbedEditing
11:52:36.873 plugincollection.js:255 instantiatePlugin MediaEmbedUI
11:52:36.874 plugincollection.js:255 instantiatePlugin AutoMediaEmbed
11:52:36.874 plugincollection.js:255 instantiatePlugin Table
11:52:36.874 plugincollection.js:255 instantiatePlugin TableEditing
11:52:36.874 plugincollection.js:255 instantiatePlugin TableUtils
11:52:36.875 plugincollection.js:255 instantiatePlugin TableUI
11:52:36.875 plugincollection.js:255 instantiatePlugin TableToolbar
11:52:36.875 plugincollection.js:255 instantiatePlugin ƒ ( editor ) {
			editor.model.schema.extend( 'tableCell', {
				allowAttributes: 'style'
			} );

Every constructor has been called in the proper order:

ArticlePluginSet
	Essentials
		Clipboard
		Enter
		ShiftEnter
		Typing
			Input
			Delete
		Undo
			UndoEditing
			UndoUI
	Autoformat
	BlockQuote
		BlockQuoteEditing
		BlockQuoteUI
	Bold
		BoldEditing
		BoldUI
	Heading
		HeadingEditing
			Paragraph
		HeadingUI
	Image
		ImageEditing
			Widget
		ImageTextAlternative
		ImageTextAlternativeEditing
		ImageTextAlternativeUI
			ContextualBalloon
	ImageCaption
		ImageCaptionEditing
	ImageStyle
		ImageStyleEditing
		ImageStyleUI
	ImageToolbar
		WidgetToolbarRepository
	Italic
		ItalicEditing
		ItalicUI
	Link
		LinkEditing
		LinkUI
	List
		ListEditing
		ListUI
	MediaEmbed
		MediaEmbedEditing
		MediaEmbedUI
		AutoMediaEmbed
	Table
		TableEditing
		TableUtils
		TableUI
	TableToolbar
ƒ ( editor ) {
	editor.model.schema.extend( 'tableCell', {
		allowAttributes: 'style'
	} );

The problem with this is that this function does all the things in the constructor - it does not have the init() and afterInit() methods are called in "before parent" order:

Clipboard init
Enter init
ShiftEnter init
Input init
Delete init
Typing init
UndoEditing init
UndoUI init
Undo init
...
ArticlePluginSet init

Clipboard afterInit
Enter afterInit
ShiftEnter afterInit
Input afterInit
Delete afterInit
Typing afterInit
UndoEditing afterInit
UndoUI afterInit
Undo afterInit
...
ArticlePluginSet afterInit

So we cannot extend non-existing schema item in function() plugin.

This unveils a problem with features implementations to be sure. The function() plugins are useless for this scenario.

Most of the plugins have similar schema of defining things:

  1. The constructor() does if necessary editor.config.define()
  2. The init() method defines schema, conversion and commands.

The non-conventional are: FontBackgroundColorEditing, FontColorEditing, FontSize - does some or all things in the constructor (config, schema, commands and/or conversion).


Gathering it all together I think that the we should:

  1. Refactor Font* plugins in order to make them work as all other plugins.
  2. Think about the order of initialization of the function() plugins.

Ad. 2.: In most of the cases developers would expect that other features are loaded and fully setup. So maybe they should be called not in the constructor pipeline but rather in the init (or even afterInit) chain.

Other way is to define the init() and afterInit() methods in the function (and add such sample to the plugin documentation):

ClassicEditor
	.create( document.querySelector( '#editor' ), {
		plugins: [ ArticlePluginSet, function( editor ) {
			this.init = () => {
				editor.model.schema.extend( 'tableCell', {
					allowAttributes: 'style'
				} );
			};
		} ],

//cc @pjasiun / @oleq

@oleq
Copy link
Member

oleq commented Jun 18, 2019

Wouldn't the following help

ClassicEditor
	.create( document.querySelector( '#editor' ), {
		plugins: [
			ArticlePluginSet,
			function( editor ) {
				this.init = () => {
					editor.model.schema.extend( 'tableCell', {
						allowAttributes: 'style'
					} );
				}

				editor.conversion.attributeToAttribute( {
					model: {
						name: 'tableCell',
						key: 'style'
					},
					view: 'style'
				} );
			}
		],

? AFAIR all plugins, even plain functions are instantialized.

@jodator
Copy link
Contributor

jodator commented Jun 18, 2019

@oleq yes - that's one of the proposed solutions in my lengthy post :D

If we agree that the this.init = () =>{} is enough I'll document this treat in the docs properly as it is not so obvious. Probably if one learn from the docs that the plugin can be simple function then he/she would also should learn about proper init() setup.

Right now this might be counter-intuitive and might be changed so the function() plugin will be treated as init() not as constructor(). So at least we should document this.

oleq referenced this issue in ckeditor/ckeditor5-core Jul 30, 2019
Docs: Added a note to the `PluginInterface` about the limitations of plugins defined as plain functions. Closes #175.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-core Oct 9, 2019
@mlewand mlewand added this to the iteration 26 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. type:improvement This issue reports a possible enhancement of an existing feature. package:core labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:core type:bug This issue reports a buggy (incorrect) behavior. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants