kefirjs / kefir

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

Map vs Scan pooling #253

Closed ivan-kleshnin closed 6 years ago

ivan-kleshnin commented 6 years ago
let xs = K.pool()
let ys = xs.map(x => x * 2)

xs.plug(K.constant(1))
xs.plug(K.constant(2))
xs.plug(K.constant(3))

ys.observe(console.log) // 2--4--6

3 events are emitted.

let xs = K.pool()
let ys = xs.scan(x => x * 2, 1)

xs.plug(K.constant(1))
xs.plug(K.constant(2))
xs.plug(K.constant(3))

ys.observe(console.log) // 6

1 event is emitted.

This doesn't feel right. If pool queues values before the first subscription – both cases should emit 3 events. If pool keeps only the last value before subscription – why 3 events in the first case?

rpominov commented 6 years ago

This is because scan always produces a Property, which has a difference from Streams described in #142 .

You're right that this feels wrong. I'm not sure how to fix this properly though. Options are:

  1. Change scan so it would create a Stream if source is a Stream (breaking change)
  2. Do #142 (breaking change)
  3. Add toStream method, that would work like changes, but keep "current" values (And also recover current values consumed by Properties somehow? Not sure this is even possible)

None of these solution seem 100% right and noncontroversial to me. Sometimes I think the core conceptual problem is Properties. I mean maybe overall semantics of the library would be more solid and consistent if we simply had only Streams.

Also I should mention that having multiple "current" values is not recommended anyway. You may have other issues with such pattern. I'd try to avoid it.

ivan-kleshnin commented 6 years ago

None of these solution seem 100% right and noncontroversial to me.

Yes, reading those issues, I see what you mean.

I mean maybe overall semantics of the library would be more solid and consistent if we simply had only Streams.

I guess it depends on how often (and how different) Properties are used in the app code.

In RxJS I use .shareReplay(1) only for state observables which are exceptions among the other streams. XStream reinvented the same property concept as MemoryStream. Intuitively, it seems it's just a matter of adding a single operator like https://github.com/mostjs/hold. I may be wrong here, but my reasoning is the following:

So yes, I would, at least, try to remove Property and replace it with an operator which you have to manually inject in the places where you want to track the events (the last one is the most useful, the last two are occasionally useful, the uncontrollable N is questionable but why not).

rpominov commented 6 years ago

Yeah, a simple operator like hold sounds like something that may work better that our current Properties semantics. But if we change that it'll be very painful for people to migrate their code bases. So I consider this more like a theoretical discussion at this point :)

ivan-kleshnin commented 6 years ago

I understand. Maybe like an idea for Kefir 4.x, for the future. I'll close this issue then, because it's not unique. A note for possible readers who still don't get it:

You should observe before pushing values to streams synchronously