paldepind / flyd

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

Inconsistent behavior when creating / manipulating streams inside a stream body. #160

Closed m59peacemaker closed 6 years ago

m59peacemaker commented 6 years ago
    const n = flyd.stream(1)
    const nPlus = n.map(v => v + 100)
    console.log(nPlus()) // 101

    flyd.stream(1).map(() => {
      const n = flyd.stream(1)
      const nPlus = n.map(v => v + 100)
      console.log(nPlus()) // undefined
    })

Running the same code within the body of a flyd stream behaves differently than from without. This created a nightmarish bug for me to track down and I can't see a way to get around it.

StreetStrider commented 6 years ago

This looks like a bug, but also may be caused by guarantees which are provided by flyd. Can't say for sure, did you investigate the code?

m59peacemaker commented 6 years ago

I do believe it is because of the atomic updating. It might require a major rewrite to fix this, if it's possible at all.

garkin commented 6 years ago

Could you please test it with the following version: npm install https://github.com/garkin/flyd/tarball/v0.3.2-forked

m59peacemaker commented 6 years ago

@garkin It has the same issue. I've written a function to handle atomic updating that doesn't have this issue, but I have unsettled questions about how end streams should work during an atomic update. Quite a headache trying to figure it all out.

garkin commented 6 years ago

My bad, was out of scope for a long time. I should've seen it from the beginning.

Yes, existing realization does not guarantee synchronous resolve of the streams current values and flyd.on() should always be used for side effects. This actually conflicts with the primitive examples from readme.md.

I came to flyd from Bacon.js and it's not an issue there, since there is no concept of the synchronously available stream current value, only .combine() and .on() counterparts. I believe it's the same for rxjs and others. So it won't be idiomatic to rely on it.

Language- and library- agnostic-wise Reactive Programming is an Asynchronous Pattern.

Anyway, looking forward to your solution.

m59peacemaker commented 6 years ago

I believe this is the test demonstrating the same behavior https://github.com/paldepind/flyd/blob/master/test/index.js#L175

Seems intentional, but I wish I knew why...

m59peacemaker commented 6 years ago

I think this is related, too: https://github.com/paldepind/flyd/blob/master/module/switchlatest/index.js#L7-L9

The nested combine doesn't fire immediately when its dependency stream has a value, but combine normally does (and the outer one does).

paldepind commented 6 years ago

@nordfjord has submitted a solution to this problem. There is a new release on npm that contains the fix. Please test that it works as expected. Note that a dependent stream created in response to an event in another stream is only updated after that event has finished propagating.

paldepind commented 6 years ago

This fix is now on npm 😄