paldepind / flyd

The minimalistic but powerful, modular, functional reactive programming library in JavaScript.
MIT License
1.56k stars 85 forks source link

scan, scanMerge inconsistent initial behavior #161

Open m59peacemaker opened 6 years ago

m59peacemaker commented 6 years ago

When using scan on a stream, if the stream does not yet have a value, the initial value is set on the returned stream. scanMerge also uses the initial value as the first (and immediate) value of the returned stream. However, scan will use the given stream's value immediately (as the initial value), if that stream does already have a value, but scanMerge doesn't care about the state of the streams it is scanning/merging, and always uses the initial value initially. Is it intentional that scan and scanMerge handle this differently or is one of them wrong?

garkin commented 6 years ago

If you want to get the same behavior from .scan() - wrap it like that flyd.immediate(flyd.scan(...)).

m59peacemaker commented 6 years ago

I don't think that's relevant to the issue. immediate will cause the combine function to be evaluated even if the dependencies are not ready (have a value), but the inconsistency I'm referring to occurs when the dependencies are ready. When the dependencies are ready when scan is initialized, it uses them, scanMerge doesn't. With scanMerge, it won't use the dependencies until one of them emits later.

m59peacemaker commented 6 years ago

Here's an example where both operators are used in the same way, but produce different initial results.

const x = scan
  ((acc, v) => acc + v)
  (0)
  (stream(345))
x() // 345

const y = scanMerge
  ([ [ stream(345), (acc, v) => acc + v ] ])
  (0)
y() // 0
m59peacemaker commented 6 years ago

Also note that merge would immediately reflect the value of the first merged stream that has a value, so both scan and merge use their dependencies at initialization, but scanMerge does not. There's no changed dependencies on the initial call, so it just returns the initialValue. It's easy to change this, but I don't know if it's intentionally different. https://github.com/paldepind/flyd/blob/master/module/scanmerge/index.js#L13-L14