kefirjs / kefir

A Reactive Programming library for JavaScript
https://kefirjs.github.io/kefir/
MIT License
1.87k stars 97 forks source link

Synchronous emit in flush method of debounce produces incorrect result #307

Closed betalb closed 3 years ago

betalb commented 4 years ago

If you will try to do synchronous emit in debounced observable incorrect sequence of events will be produced

i.e.

const a = Kefir.pool();
const b = a.debounce(100).spy();
a.plug(b.filter((val) => val === 1).map(() => 2));
a.plug(Kefir.constant(1));
a.observe();

Expected output

100ms [pool.debounce] <value> 1
200ms [pool.debounce] <value> 2

Actual output

100ms [pool.debounce] <value> 1
200ms [pool.debounce] <value> null
mAAdhaTTah commented 4 years ago

Without having investigated, the issue is probably less about synchronous emit and more about the circular dependency between a & b.

betalb commented 4 years ago

yes, there is a cycle, but it is not infinite and is actually done to illustrate an issue

Here is another more dirty example that produces the same result

let e;
Kefir.stream((emitter) => { e = emitter; })
  .debounce(100)
  .withHandler((emitter, event) => {
    if (event.type === 'value' && event.value === 1) {
      e.emit(2);
    }
    emitter.emitEvent(event)
  }).log();
e.emit(1);

Actually I created PR that should highlight where the issues seems to be. And there are similar problems in buffer-by, buffer-while-by (I just wanted to create cleaner test case before creating separate ticket)

rpominov commented 4 years ago

This is documented behaviour:

image

Although documentation might be more clear probably. My idea was back then is that synchronous events is not a use case I wanted to support well in the library in general.

betalb commented 4 years ago

@rpominov I'm not following documentation quote, for me it sound like a different case, which I never faced 🙂

This issue is not with the fact that some value is emitted without being debounced, but completely wrong value (null) being emitted

Also I've provided 2nd example that doesn't use properties and here is 1st example without using properties either

const a = Kefir.pool();
const b = a.debounce(100).spy();
a.plug(b.filter((val) => val === 1).map(() => 2));
a.plug(Kefir.stream(emitter => { emitter.emit(1) }));
a.observe();
mAAdhaTTah commented 4 years ago

@rpominov The issue isn't that it's emitted synchronously (it actually isn't, according to the issue description), but that the second event is dropped.

rpominov commented 4 years ago

Ah, right, sorry

bpinto commented 3 years ago

Hey guys, @betalb opened a PR fixing this, do you think we could merge their PR?

mAAdhaTTah commented 3 years ago

@bpinto Done. Let me get a release out this weekend.