Events system

[Reinmar]

There may be couple of things that we would like to improve. Things I remember:

1. Callback's return value should not affect the event. Currently if a callback returns false, then the event is cancelled. This is wrong for two reasons:
   • you may mistakenly register a listener than returns false but without an intention of cancelling the event; for example it may be some object's method.
   • it should be clear that listener cancels an event - someone reading the code might not associate returning false with cancelling it or may just miss it.
2. Currently if you try to register one function twice as a listener for the same event, the second listener will be ignored with no error whatsoever. E.g. https://gist.github.com/oleq/7afd10c0be0f76fb1b98/35de7da0b2e2307873520531f81b5f32723cd0de#file-eventtrap-js-L2. Error should be thrown or multiple listeners should be accepted.
3. Performance. Events system is a base of v4 and will be used even more in v5. Even small improvements will have a global effect.

[oleq]

So 👍! The event system of v3+ is very obscure.

[adelura]

If we are going to provide MV*, we should also implement listenTo and stopListening functions. Implementation doesn't change current API but extend it. The idea is that events are keept also in object who is listening, so when destroying it, we can easily unbind all listeners. Backbone libarary has it's own implementation, and it's described for example here.

[wimleers]

The idea is that events are keept also in object who is listening, so when destroying it, we can easily unbind all listeners. Backbone libarary has it's own implementation, and it's described for example here.

This turned out to be vitally important for fixing Backbone memory leaks. CKEditor is a much more tightly controlled project, so it's probably okay to rely on institutional knowledge on how to prevent memory leaks, but still, it'd be better if it were done explicitly for you, and would probably help ensure that 3rd party CKEditor plugins also don't have memory leaks :)

[fredck]

Following the above comments, which make sense, a general purpose events API is to be developed.
The Emitter mixin will hold it (note the UpperCamelCase notation for mixins).

The following is the interface of it:

• on( event, callback, [ctx], [priority] ) // differs from v4
• once( event, callback, [ctx], [priority] ) // differs from v4
• off( event, callback ) // removeListener() in v4
• fire( event, [args] )

And the new stuff (check Backbone):

• listenTo( emitter, event, callback, [ctx], [priority] )
• stopListening( [emitter], [event], [callback] )

fire() will pass ( event, args ) to the callbacks, where event is an instance of EventInfo with the following interface:

• name // the event name
• stop() // no further callbacks called
• off() // removes the current callback from future event calls

Comments?

[adelura]

Since we will have tree structure of models (thanks to collections) it would be powerfull to have events propagations just like in DOM.

I also recommend you to fire event on object by this object itself. i.e. this.fire(). In other words fire method should be treated as a private one.

[fredck]

Since we will have tree structure of models (thanks to collections) it would be powerfull to have events propagations just like in DOM.

That's very interesting... but the problem is that, while the DOM deals with the same kind of objects in the tree (elements), we deal with totally different objects.

For example: editor -> toolbar -> button. Let's suppose that "button" fires a "open" event, which is present also on "toolbar" and "editor" with totally different semantics. So if I listen to "open" in "editor", it would mean three different things, depending on what is the source of the event.

In the DOM this is much clearer... if one element fires "click", this event still makes sense for its whole parent three because all parents can fire "click" with the very same semantics.

So I'm a bit unsure about it still.

I also recommend you to fire event on object by this object itself. i.e. this.fire(). In other words fire method should be treated as a private one.

This is usually true... but it happens very often that one wants to simulate or force a behavior in an object, firing an event of it manually. So, IMHO, we should not enforce this to be private.

[adelura]

For example: editor -> toolbar -> button. Let's suppose that "button" fires a "open" event, which is present also on "toolbar" and "editor" with totally different semantics. So if I listen to "open" in "editor", it would mean three different things, depending on what is the source of the event.

I don't say that this pattern might work in all scenarios. Please note that in v5 we will have a lot of various models and collections, which we don't think about yet. This pattern might work even in toolbar > button scenario: https://gist.github.com/adelura/d72870b378fcf64e91fe Imagine that you have to add listener to each button when added and remove listener when button is removed.

So, IMHO, we should not enforce this to be private.

Yep, but it might be just coding guideline

[fredck]

This pattern might work even in toolbar > button scenario

@adelura, please note that I'm totally with you in this feature. I really like the idea... still I see problems.

Ofc, the case you exemplified looks like the perfect world... but let's take the other case, when I don't want to listen to child events.

Let's start with the DOM... When you listen for 'click' in an element (PARENT), it is ok to have it fired by a child (CHILD) element because while the 'click' happened in CHILD, it also happened (semantically) in PARENT. So if want to listen for 'click' in PARENT, I'm fine... and if I want to use PARENT to listen to 'click' in all CHILDs of a certain type, then I'm fine again.

Now back to our case... let's suppose that we have the 'open' event in toolbar, which is fired when the toolbar opens after being collapsed (e.g.). Then let's suppose that I have 'open' in buttons as well, which is fired when a button opens a dialog (e.g.). If I do toolbar.on( 'open' ), what I'm listening to? If I want to catch the toolbar 'open', I don't have to care about checking in the source of the event is really the toolbar. Actually, I may not even be aware that buttons fire 'open' for different purposes as well. Confusion and WTFs in place because of this.

I'm still a bit unsure about it because of all this.

[adelura]

DOM is a great example how event propagation should work :) And I agree that not always custom event propagation is semantically correct.

Confusion and WTFs in place because of this.

Let me fix this up. See improved version https://gist.github.com/adelura/d72870b378fcf64e91fe I also introduced event static namespace which is pinned to constructor function. It brings readability. But what is more important is that default behaviour is the same as previous ones. However passing fourth argument into on method change it's default behaviour.

[fredck]

@delura, to summarize, you're proposing to introduce a new parameter to on(), so it'll look like this:

on( event, callback, [ctx], [priority], [sourceType] )

Then only if sourceType is defined we enable propagation.

That's the one and only way to have it. Curious to hear others what they think about this feature in general.

[fredck]

Btw, do we want to change the method signature in this case to something like this?

on( event, callback, ctx, options )

So option would be something like this: { priority: 10, type: Button }.

[pjasiun]

I think we use priority far more often then ctx so it should be:

on( event, callback, [priority], [ctx], [sourceType] )

Also multiple times I needed to bind one callback to multiple events so it would be useful if I could do it:

editable.on( [ 'copy', 'cut' ], function( evt ) {
   //....
} );

[fredck]

I think we use priority far more often then ctx

I see that we use both... maybe priority a bit more often. Anyway, the proposed order doesn't come out of nothing. It is the common order found out there for similar APIs. The first 3 arguments are usually in that order. "priority" is something that we invented, that's why it went to after it. Finally, we've had this in this way since forever so I'm unsure it is worth changing such detail.

[fredck]

Also multiple times I needed to bind one callback to multiple events

I see this as a edge case, but it is doable. would just like to avoid bloating this API with features because this will be extensively used in our code. So here, less is better.

Opinion from others is welcome.

[fredck]

Talking about passing context to functions that receive callbacks... shouldn't we get rid of this, considering that one can now use bind()?

[gregpabian]

Talking about passing context to functions that receive callbacks... shouldn't we get rid of this, considering that one can now use bind()?

And how exactly would you remove a listener then? You'd have to keep a reference to the bound function somewhere, while passing a regular function and the context would make the removal much easier.

[fredck]

And how exactly would you remove a listener then? You'd have to keep a reference to the bound function somewhere, while passing a regular function and the context would make the removal much easier.

That makes sense. So we can take this inconsideration only for methods that accept callback but are note paired with a remove() method for that callback. It means that this should not apply to the events API at least.

[Reinmar]

I see this as a edge case, but it is doable. would just like to avoid bloating this API with features because this will be extensively used in our code. So here, less is better.

Not so edge IMO - I saw this repeating, especially in UI related code (recalculate something when whatever changed). And it's pretty simple to allow doing:

obj.on( 'foo,bar,bom', ... )

or more explicitly with an array of names.

[Reinmar]

And how exactly would you remove a listener then? You'd have to keep a reference to the bound function somewhere, while passing a regular function and the context would make the removal much easier.

Actually, we have a feature in CKE4 that I like - the on() method returns an object with a removeListener() method. I find this much better solution that using off() will all the arguments again. Note that all the arguments passed to on() must be passed to off() what leads to ugly code like:

obj.on( 'foo', x.onFoo, x, -1 );
obj.on( 'bar', x.onBar, x, 999 );
obj.on( 'bom', x.onBom, x );

// later on:

obj.off( 'foo', x.onFoo, x, -1 );
obj.off( 'bar', x.onBar, x, 999 );
obj.off( 'bom', x.onBom, x );

I like this much more:

var listeners = [
    obj.on( 'foo', x.onFoo, x, -1 ),
    obj.on( 'bar', x.onBar, x, 999 ),
    obj.on( 'bom', x.onBom, x )
];

// later on:

_.invoke( listeners, 'off' );

This is less important once you have listenTo() and stopListening(), because we needed multiple-off calls usually when destroying some object (but not only). But I still think that this is a pretty useful feature. The code is cleaner and most likely it's also more performant approach (no listeners lookup).

[Reinmar]

DOM is a great example how event propagation should work :) And I agree that not always custom event propagation is semantically correct.

That's the one and only way to have it. Curious to hear others what they think about this feature in general.

I'm not a big fun of events propagation in the model or views. In my understanding there's a significant difference between DOM events and events in views. Let's consider a hypothetical structure:

<div class="toggleablePanel">
   <p>Some text. <a href="" class="helpIcon">?</a></p>
</div>

Now, let's assume that click in the panel should fold it, but click in the help icon should show something else.

If you listen on toggleablePanelView#click you want to be sure that the click means "the panel was clicked". Not that "something in the panel was clicked". So if events from the helpIconView view were propagated to toggleablePanelView you would always need to remember about filtering the target. Additionally, the helpIconView does not know if the event should be propagated, because it's its parent decision, so it cannot decide to do this when firing the event.

Also, note the semantical difference. Click on the panel means that you want to close it and click on the icon means that you want the help. DOM does not know about this, hence the propagation in its case makes sense.

Last but not least, there must be an association between views to make propagation possible and only views know about it. So if we'll implement propagation, it will need to be integrated with the classes on which it happens. Hence, no need to think about this in relation to the general events API.

[Reinmar]

I'm not a big fun of events propagation

However, I am a big fun of useful functions. Two One that came to my mind was:

obj#fwd( emitter, event(s), [priority], emitOn, emitWhat )

E.g. (view#click will cause firing parent#click - explicit propagation):

view.fwd( view, 'click', null, view.parent, 'click' );

or, very useful events merging (all 3 events will cause firing view#layoutChange):

view.fwd( view, [ 'resize', 'scroll', 'move' ], view, 'layoutChange' );

[Reinmar]

Actually, instead of:

view.fwd( view, 'click', null, view.parent, 'click' );

we can do:

view.on( view, 'click', 'click', view.parent );
view.on( view, [ 'resize', 'scroll', 'move' ], 'layoutChange' );

So if the listener is a string it's an event name to be fired.