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

Do we need support for asynchronous components initialisation? #5355

Closed
Reinmar opened this issue May 9, 2017 · 2 comments · Fixed by ckeditor/ckeditor5-ui#264
Closed

Do we need support for asynchronous components initialisation? #5355

Reinmar opened this issue May 9, 2017 · 2 comments · Fixed by ckeditor/ckeditor5-ui#264
Assignees
Labels
package:ui status:discussion type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented May 9, 2017

Long, long time ago we assumed that our UI framework needs to support async components. An example of an async component is one which uses an iframe. For it to be useful, ready to work, we need to wait for the iframe's load event. Otherwise, we wouldn't be able to put child components inside this one (if it had some collection).

So, examples of async components are, for instance:

  • iframed editable,
  • iframed dropdown panel list (used extensively in CKE4, not used at all in CKE5, but may be needed one day),
  • some rich UI component which requires lazy loading (big colorpicker widget :D).

In order to support this, we split component initialisation into two parts – the constructor and potentially async init() (it can return a promise). We also allowed destroy() to be asynchronous (just in case). All this meant that ViewCollection.add/destroy() are asynchronous too. (I noticed that remove() isn't async... which may be a bug in this plan, cause removing an iframe unloads it, so it's close to destroy :D).

It'd be good if this was where the asynchronous execution would end. But if one of your methods returns a promise then you need to carry it around everywhere and all your code needs to deal with an asynchronous execution. I report this ticket exactly because we noticed that a high level component (ContextualBalloon) has a problem with it.

Do we need this at all?

The first question which we should ask immediately is – do we need all these problems at all? Do we really need to support asynchronous components initialisation?

The idea behind creating our own UI framework was that we have a very limited and quite specific use case so we might be able to come up with something specialised and simple at the same time. Async support works against this. It heavily complicates our code.

At the same time, we don't have any async component yet and, even though we implemented iframed editable in the past to see how the concept works, I'm pretty sure that we missed something (just like the remove() issue).

So, perhaps we can drop support for async initialisation which will allow simplifying the code significantly. It will not only affect the UI lib but also features.

But what about those special, rare cases where we really need to deal with asynchronousity?

Who should be concerned about component's state?

Disclaimer: It may be a total BS what I'll write here because I didn't do any research and I haven't tested this concept, but let's see.

The idea about Web Components is dear to me. It assumes that every component is totally independent which makes it reusable and allows easy composition. Components should have, therefore, a clear and simple API. Just like the entire DOM.

But how does DOM handle asynchronousity? Well, it doesn't (I'm not sure about Web Components, though. @Comandeer?).

If you created an <iframe> and added it to the DOM using appendChild() it's your duty to postpone any further execution until the load event. appendChild() doesn't return a promise too. Nothing like that.

This means that the platform is simple (yeah... DOM is so simple!) and the potential complexity is shifted to its users. After all, how often do you deal with iframes? Why making the entire API async if you need to deal with async cases once in a lifetime?

Let's also look on this from a different angle. If we have ContextualBalloon which delegates the add/remove API of its internal container, then who really needs to care about the fact that an iframe added to the ContextualBalloon is not ready immediately? Certainly not the ContextualBalloon because it only cares about <iframe> being its child. It's the code which creates that iframe component and uses it. So it's the caller, not the callee.

So, tl;dr here is that we made sure that the entire tree of components and the API needs to be aware of the state of some component that may be added to it. But we should do the exact opposite – save everyone from this knowledge and only make the smallest possible piece of code (usually, some glue code or controllers) aware of it.

@oleq
Copy link
Member

oleq commented May 9, 2017

The longer we maintain this out-of–the–box async UI component support, the more I consider it a burden rather than a useful feature. It complicated tons of tests in the past and we haven't seen a single benefit of it yet.

The problem is especially visible when some async code is executed in sync event listeners like render. If such event is fired frequently, new promises are created before old ones actually finish their lifetime and because there's no mechanism to manage such processes, we end up with errors and odd behaviors (like in ContextualBalloon). Even if we decided to build such mechanism, it would be complex and it would solve no actual issue at this moment because we don't use async UI components at all.

Certainly, getting rid of it will shorten and simplify lots of code (and tests).

@oleq
Copy link
Member

oleq commented May 10, 2017

Another example of the problem our async–driven API has created: the destruction of the editor, mostly in tests.

Both UI initialization and destruction processes are chains of Promises. They work fine but when the editor is already running, there's no (easy) way to synchronize things like ViewCollection#add (which is async) with sudden destruction of the editor. So if a test performs lots of async UI actions (like adding and removing in VCollection) and then the test runner destroys the editor, it usually ends up with errors because the editor (in fact, any piece of code outside the component being initialized) has no idea that some promises await their turn and this some UI actions are performed when the editor is not in DOM any longer.

To handle this issue, we'd need to create an UI structure, sort of queue, which would inform the outside world about upcoming operations and make it wait for them to finish, e.g. before destruction or something like this. It feels like a lot of work with no clear benefit and, besides, the tests would still remain very complex.

@oleq oleq self-assigned this Jul 3, 2017
Reinmar referenced this issue in ckeditor/ckeditor5-ui Jul 5, 2017
Other: Made the UI component initialization and destruction processes synchronous. Closes #225.

BREAKING CHANGE: `View#init()`, `View#destroy()` (also `ViewCollection#init()`, `ViewCollection#destroy()` and `ViewCollection#add()`) no longer return `Promise`. It may require updates in UI components which inherit from `View` and rely on the value returned by these methods.

BREAKING CHANGE: Various UI components switched to synchronous `init()` and `destroy()` (no longer returning `Promise`), which means that features using these components may need some updates to work properly.
Reinmar referenced this issue in ckeditor/ckeditor5-editor-balloon Jul 5, 2017
Reinmar referenced this issue in ckeditor/ckeditor5-core Jul 5, 2017
Reinmar referenced this issue in ckeditor/ckeditor5-editor-inline Jul 5, 2017
Reinmar referenced this issue in ckeditor/ckeditor5-editor-classic Jul 5, 2017
Reinmar referenced this issue in ckeditor/ckeditor5-image Jul 5, 2017
Reinmar referenced this issue in ckeditor/ckeditor5-link Jul 5, 2017
Reinmar referenced this issue in ckeditor/ckeditor5-upload Jul 5, 2017
oleq referenced this issue in ckeditor/ckeditor5-theme-lark Jul 6, 2017
…iew() helper became synchronous (see ckeditor/ckeditor5-ui#225).
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-ui Oct 9, 2019
@mlewand mlewand added this to the iteration 11 milestone Oct 9, 2019
@mlewand mlewand added status:discussion type:improvement This issue reports a possible enhancement of an existing feature. package:ui labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:ui status:discussion type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
3 participants