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 can not set data, a.k.a. editor.instanceReady #2869

Closed
pjasiun opened this issue Nov 17, 2016 · 11 comments · Fixed by ckeditor/ckeditor5-core#48
Closed

Plugins can not set data, a.k.a. editor.instanceReady #2869

pjasiun opened this issue Nov 17, 2016 · 11 comments · Fixed by ckeditor/ckeditor5-core#48
Assignees
Labels
package:core type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@pjasiun
Copy link

pjasiun commented Nov 17, 2016

Now, the order of initialization is:

editor.initPlugins()
   .then( () => editor._elementReplacer.replace( element, editor.ui.element ) )
   .then( () => editor.ui.init() )
   .then( () => editor.editing.view.attachDomRoot( editor.ui.editableElement ) )
   .then( () => editor.loadDataFromEditorElement() )
   .then( () => editor )

It means that plugins are init before the loadDataFromEditorElement, which calls setData. What if a plugin wants to setData?

But wait, there is more! What if a plugin wants to modify initial data or just read them? Plugins are also initialized before the ui is initialized. What if a plugin wants to use the UI?

So there could be an event that... the instance is ready, so plugins can execute some actions on it.

Alternatively, there could be a second method in plugins called when editor is ready. Thanks to it plugins could return a promise in this method what is not possible in events.

Another solution is to check if there is not data already loaded before calling loadDataFromEditorElement. It would solve only the fist problem, but it is my favorite solution because is the least intrusive and seems to be the most correct behavior (hacks-less).

@Reinmar
Copy link
Member

Reinmar commented Nov 17, 2016

We definitely miss a lot of events now. I've also spotted https://github.com/ckeditor/ckeditor5-heading/issues/40 some time ago. I guess we can add few basic events like after plugin init, on ready.

But in fact, your request is a bit strange. Why would a plugin load data? I guess that you're trying to integrate CKEditor with its surrounding by defining a plugin... which isn't necessarily correct. Mind that if your plugin would listen on editor#ready and load some data, that wouldn't prevent from loading some other data before (in the classic editor case). Perhaps you just need a different Editor.create() method which doesn't call loadDataFromEditorElement(), but then you can also easily fire there an event on which your plugin will listen :P.

In other words – you reported a ticket which would be valid in CKE4 world. In CKE5 world, if you need this for your integration, just write a better create() method.

But this of course doesn't change the fact that by default we could fire a few events.

@Reinmar Reinmar self-assigned this Nov 29, 2016
@Reinmar
Copy link
Member

Reinmar commented Nov 29, 2016

I'll add two events editor#ready and editor#pluginsReady and editor#dataReady and editor#uiReady. And we already have editor#destroy.

@pjasiun
Copy link
Author

pjasiun commented Nov 29, 2016

I'll add two events editor#ready and editor#pluginsReady and editor#dataReady and editor#uiReady.

We need to talk about counting.

@Reinmar
Copy link
Member

Reinmar commented Nov 29, 2016

This feature is required for https://github.com/ckeditor/ckeditor5-heading/issues/40. And will require small changes in the classic creator.

@Reinmar
Copy link
Member

Reinmar commented Dec 1, 2016

I've just realised that we could also have Plugin.aferInit() like in the good old days. It'll be easier to use.

@Reinmar
Copy link
Member

Reinmar commented Dec 1, 2016

I've been also thinking about the beforeInit() (which we have in CKEditor 4) and the plugin constructors (which we have in CKEditor 5).

I see that we use init() everywhere and not the constructors. First I thought that this is a mistake and we should use the constructor() in the heading feature in order to fix https://github.com/ckeditor/ckeditor5-heading/issues/40.

However, there's a reason why we used init(). It was simply more natural. Perhaps there's no sense in trying to force everyone to use constructors() as the main initialization point, especially that then we wouldn't have a space for beforeInit().

So, I want to propose that constructor is the "before init" (to be used in special cases), init() is the typical init point and there's also afterInit() (to be used in special cases).

WDYT?

@pjasiun
Copy link
Author

pjasiun commented Dec 1, 2016

That's a very cool idea. Especially the afterInit. Since all plugins add converters in the init, there are actions you want to handle later, when all converters are there. beforeInit might be useful too.

I'm thinking if it should be a method or an event. The profit of having a method is that you can return a promise there and we could handle it.

On the other hand, it might be unclear what beforeInit means. I mean, it is of course before init and if one plugin wants to do something before any other plugin, it should use beforeInit, but what if the second plugin wants to do something before any other plugin? How can one plugin be sure that there is no another plugin using beforeInit and what that plugin is doing in such method/event?

@Reinmar
Copy link
Member

Reinmar commented Dec 1, 2016

On the other hand, it might be unclear what beforeInit means. I mean, it is of course before init and if one plugin wants to do something before any other plugin, it should use beforeInit, but what if the second plugin wants to do something before any other plugin? How can one plugin be sure that there is no another plugin using beforeInit and what that plugin is doing in such method/event?

You're going too far :D. There's no "I want to do something before anyone else do anything". I don't recall a case from CKEditor 4 in which that would be a problem. >90% of the code lands in init() and there are special cases for beforeInit() and afterInit() in CKEditor 4.

And, to make it clear, in CKEditor 5 I don't want to have beforeInit() because we already have plugin constructors. So CKE5's Plugin.constructor === CKE4's PluginDef.beforeInit.

@fredck
Copy link
Contributor

fredck commented Dec 2, 2016

If no beforeInit, I think that an event solution would fit better. In this way we have many different points in time during the editor loading that can be listened by plugins, in the very same way. It would make the code clearer, without special cases.

@Reinmar
Copy link
Member

Reinmar commented Dec 5, 2016

As I pointed out in ckeditor/ckeditor5-core#48 (comment) there's no simple way to have a separate beforeInit (neither as an event nor as a method), so I assumed that the plugin constructor (which obviously executes before plugin's init()) is the new beforeInit. That's the simplest solution I saw.

@fredck
Copy link
Contributor

fredck commented Dec 5, 2016

I see that my comment was very unclear... I was talking about afterInit... I've also commented about this in ckeditor/ckeditor5-core#48 (comment).

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-core Oct 9, 2019
@mlewand mlewand added this to the iteration 6 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed 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: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