Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Map vs Scan pooling #253

Closed
ivan-kleshnin opened this issue Oct 21, 2017 · 4 comments
Closed

Map vs Scan pooling #253

ivan-kleshnin opened this issue Oct 21, 2017 · 4 comments

Comments

@ivan-kleshnin
Copy link

ivan-kleshnin commented Oct 21, 2017

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
Copy link
Member

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 Should we allow property to emit multiple "current" values/errors to the first subscriber? #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
Copy link
Author

ivan-kleshnin commented Oct 22, 2017

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:

  • An additional operator requires to define a single additional semantics. Just that.
  • An additional entity requires to consider its semantics everywhere.

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
Copy link
Member

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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants