Don't do nested emits, use queue?

[rpominov]

Currently if events are emitted in response to other events we may have nested notification loops, which looks like this:

listenerA1(xA)
listenerA2(xA)
  listenerB1(xB)
  listenerB2(xB)
    listenerC1(xC)
listenerA3(xA)

Instead it should be something like this:

listenerA1(xA)
listenerA2(xA)
listenerA3(xA)

listenerB1(xB)
listenerB2(xB)

listenerC1(xC)

Basically it changes the order in which listeners are called. This change will allow us to avoid a certain type of bugs (for instance #119), and remove some checks in the code. Also it should make library more reliable, and "easier to reason about".

We should investigate what side effects this change may introduce, and how other libraries implement this aspect of dispatcher. Also this queue can be global or local (per each stream). If it local it won't remove all kinds of nesting, but only when an event happens in response to another event from the same stream.

[Macil]

I wonder if Bacon bug https://github.com/baconjs/bacon.js/issues/574 might be caused by an attempted solution to this problem there. That bug was why I started using Kefir more, so I just want to mention it in case there's a danger it could get re-implemented here.

[rpominov]

Thanks, edge cases like this is what worries me most about this change. I'll check for this when it comes to implementation.

[Macil]

I'm a little worried about this proposal because it causes some events to be processed synchronously and some to be processed later, and the deciding factor is effectively the current call stack itself. Often I find it important to synchronously process events: the most common case is when I'm listening to DOM events and need to call .preventDefault or .stopPropagation on the event before it has bubbled up. And DOM events can be synchronously emitted with .dispatchEvent: usually I do this in browser extensions where I want the page javascript to receive a fake event, or I do this in testing code. I'd find it concerning if that wasn't guaranteed to work with Kefir.

[rpominov]

No, all events will still be processed synchronously, it's just the order in which callbacks notified will be changed.

The emitEvent function will work something like this:

if currently_in_the_notification_loop
  add_event_to_the_queue
else
  start_notification_loop_with_this_event

And in the dispatcher will be a block after loop like this:

while queue_not_empty
  event = take_oldest_event_from_the_queue
  start_notification_loop_with_this_event

So we won't introduce asynchrony, but only change order of callbacks calls. But I agree this also can cause a lot of unexpected side effects, and we should be very careful with this change. At very least we should look at how this done in other libs, and what es-observable proposal says about this (if any). I think Bacon use local queue per each stream, and it works fine in there. But I haven't looked closely yet.

[Macil]

The emitEvent function would only sometimes execute all of the listeners before returning, depending on the current call stack itself. This code would be affected by global or local queues:

import assert from 'assert';
import Kefir from 'kefir';
import {EventEmitter} from 'events';

class Event {
  constructor(detail) {
    this.defaultPrevented = false;
    this.detail = detail;
  }
  preventDefault() {
    this.defaultPrevented = true;
  }
}

const element = new EventEmitter();
Kefir
  .fromEvents(element, 'foo')
  .onValue(event => {
    console.log('event process start', event);
    if (event.detail === 1) {
      const event2 = new Event(2);
      element.emit('foo', event2);
      assert(event2.defaultPrevented);
      console.log('success');
    } else {
      event.preventDefault();
    }
    console.log('event process end', event);
  });

element.emit('foo', new Event(1));

Current output in Kefir 2.x:

event process start { defaultPrevented: false, detail: 1 }
event process start { defaultPrevented: false, detail: 2 }
event process end { defaultPrevented: true, detail: 2 }
success
event process end { defaultPrevented: false, detail: 1 }

The above does seem like a silly example, emitting an event to itself. (When I once found myself accidentally doing that, I realized I was doing some things wrong.) I'm not too worried about local queues, but it definitely should be treated as a breaking update.

The idea of a global queue shared between all Kefir streams scares me to think about how it would affect the codebases I work on. Maybe application code that's written to be safe with global queues would be better, but it's hard to know if it is safe for that. A function can be called from a different location and mysteriously not work just because a few layers up the call stack is (or is not) inside a Kefir listener.

[rpominov]

This is a good example we should consider, but introducing events in a stream as an immediate response to another event from the same stream probably not a good idea anyway :smile:

Currently if a pattern like that is used there is another problem that listeners can get events in a different order:

listener1(1)
 listener1(2)
 listener2(2)
listener2(1)

So listener1 gets 1, 2, and listener2 gets 2, 1. This change will fix that, but will introduce problem you mentioned that after calling .emit() there is no guarantee that listeners immediately notified. So it's a trade between two problems.

Also I agree that it probably a bad idea to introduce global queue, a local one seems more reasonable.

I still don't have a strong opinion about this change, opened the issue mainly for discussion. And this will certainly be considered a breaking change.

[rpominov]

Probably won't happen closing for now to cleanup open issues.