kamicane / prime

An essential JavaScript library for node.js and browsers
154 stars 32 forks source link

Emitter: Add 'stoppable' arg to the constructor, defaults to false #35

Closed danfooo closed 10 years ago

GCheung55 commented 10 years ago

Whats the use case for this?

danfooo commented 10 years ago

It's for example for when different packages share the same emitter. The fact that one listener returns false shouldn't stop others from being called.

timwienk commented 10 years ago

The cause of the stopped execution of event listeners seems to be that mout's forEach (and also the pre-mout forEach, so it's consistent in that respect) stops iterating when a callback returns false.

I'm not sure this is intended, native Array.prototype.forEach doesn't do this, the cancelability may just be a bug rather than a feature. @kamicane will probably have an answer.

DimitarChristoff commented 10 years ago

not sure this is a good thing, relies on order of callbacks as well as breaks encapsulation of callbacks if n1 can affect ongoing event n2 should get. guess it's clever like DOMEvent but meh. whereas I can think of cases where I'd stop events, I can't think of a case where I'd like to override a stop from the outside.

for the record, old emitter code used object and for var in, no early break there--so valerio added it quite purposefully. I looked at it a while ago in view of porting to primish emitter but did not see any value and left it as it was.

On Monday, May 19, 2014, Tim Wienk notifications@github.com wrote:

The cause of the stopped execution of event listeners seems to be that mout's forEach https://github.com/mout/mout/blob/master/src/array/forEach.js(and also the pre-mout forEach https://github.com/kamicane/prime/blob/v0.3.2/array/forEach.js, so it's consistent in that respect) stops iterating when a callback returns false.

I'm not sure this is intended, native Array.prototype.forEach doesn't do this, the cancelability may just bug rather than a feature. @kamicanehttps://github.com/kamicanewill probably have an answer.

— Reply to this email directly or view it on GitHubhttps://github.com/kamicane/prime/pull/35#issuecomment-43525087 .

Dimitar Christoff

"JavaScript is to JAVA what hamster is to ham" @D_mitar - https://github.com/DimitarChristoff

danfooo commented 10 years ago

I think for the standard use case it's expected to break on returning false, because you "stop propagation" of the event. However when you're dealing with code that's not yours, you want an Emitter that doesn't let an arbitrary handler have that power.

DimitarChristoff commented 10 years ago

the idea of cancellable events in a flat, linear structure (non-tree bubbling events like in the DOMEvents) is wrong as you have no guarantees on order of added handlers and the wrong return/code can easily cause problems/side effects. propagation should be vertical, not horizontal - you are basically blocking your peers.

also, the API is not ideal - you tend to use mixin: [Emitter] which means no arg can get passed. this means you can't set the property from the outside and your class needs to have this._uncancelable = true, which is knowledge of the internals of Emitter I'd rather not have.

kamicane commented 10 years ago

@DimitarChristoff no, you do Emitter.call(this, true), even when you mixin.

We could reverse the default however, as in make it stopImmediatePropagationable rather than make it not stopImmediatePropagationable.

DimitarChristoff commented 10 years ago

reversed logic is better, opt into cancellable rather than opt out.

On Tuesday, May 20, 2014, Valerio Proietti notifications@github.com wrote:

@DimitarChristoff https://github.com/DimitarChristoff no, you do Emitter.call(this, true), even when you mixin.

We could reverse the default however, as in make it stopImmediatePropagationable rather than make it not stopImmediatePropagationable.

— Reply to this email directly or view it on GitHubhttps://github.com/kamicane/prime/pull/35#issuecomment-43632166 .

Dimitar Christoff

"JavaScript is to JAVA what hamster is to ham" @D_mitar - https://github.com/DimitarChristoff

danfooo commented 10 years ago

Alright, reversed now. It's the bigger change but it probably makes more sense this way.

GCheung55 commented 10 years ago

Please add tests.