Make .fire() return a promise about ckeditor5 HOT 9 CLOSED

This feature makes sense when we'll have async listeners (as part of this feature). Before that it's unnecessary.

Sent from my iPhone

On 04 Mar 2015, at 17:00, Frederico Caldeira Knabben [email protected] wrote:

It would be interesting to have .fire() return a promise.

This would enable:

Listeners to throw errors.
Have asynchronous listeners (Yeah!!!)
Emitters would be able to do this (e.g.):

editor.fire( 'something' )
.then( function() {
// Do something on success.
} )
.catch( function( err ) ) {
// Handle errors.
} );
—
Reply to this email directly or view it on GitHub.

from ckeditor5.

We talked recently about this idea and we realised one important thing – if it's listeners decision whether the event is sync or async, when firing it you have no idea what to do. Either you expect it to be sync and you put your code right below fire() call, or even your algorithm expects that all the listeners are synchronous so even if you know that you would need to use then() to queue following code, that wouldn't make sense.

This means that we cannot make all the events asynchronous by nature, because they will not be usable in some case.

This also means that perhaps you would need to tell fire() what's the nature of this event – whether you want it to be fully sync or fully async. Therefore, it looks for me that we need a separate API for async events. For example, simply fireAsync().

And last but not least, we would like (see this comment and following ones) the fire() method to return a value like in CKE4. This value would indicate what happened with the event – whether it was cancelled or not.

I'm not closing this ticket and the existing PR because we may need the code in the future.

from ckeditor5.

In the way it has been coded in ckeditor/ckeditor5-engine#29, it is a "hybrid". It is synchronous if all listeners are synchronous.

from ckeditor5.

In the way it has been coded in ckeditor/ckeditor5-engine#29, it is a "hybrid". It is synchronous if all listeners are synchronous.

Making it unpredictable for the caller. That's what we want to avoid. Either you fully expect that your event is async and you always use it as a promise or not. This means that we can have a separate API for this, making it absolutely clear.

from ckeditor5.

Well... I think that the emitter will never know this because it is the listener decision here.

You entered into contradiction with your own statement: "Either you expect it to be sync and you put your code right below fire() call, or even your algorithm expects that all the listeners are synchronous"... so you don't really expect any asynchronous listener, in that case just call fire().

If you see a chance of any of the listeners to be asynchronous, simply call fire().then().

It fits exactly on what you were talking about.

from ckeditor5.

Well... I think that the emitter will never know this because it is the listener decision here.

The caller is the one who knows the nature of the event. Not the listeners. So the caller should have a full control. Hence, separate API will make more sense.

Plus, in the solution that you propose, even if you want to have a synchronous event, fire() MUST return a promise and resolve it, which is an unnecessary overhead. The events API is the base of many heavy algorithms so the faster we make it, the better. And specialised things are always faster.

from ckeditor5.

I agree that developer who fires event need to be sure if it execute synchronously. I remember how many problems we had with the paste event in the CKEditor 4 which may be synchronous or not. So +1 for fire() which will be always synchronous, and introducing fireAsync, when needed, which may be asynchronous.

from ckeditor5.

fire() which will be always synchronous, and introducing fireAsync, when needed, which may be asynchronous.

Please note that having a different method name is irrelevant. Just documentation can tell event subscribers whether it is expected to be sync or async.

from ckeditor5.

Moved to https://github.com/ckeditor/ckeditor5-utils/issues/1

from ckeditor5.