Fixes an issue where a dependent stream is never updated

[nordfjord]

as mentioned in #162

The problem is that when dependent streams are created in the context of other streams updateStream pushes the streams to toUpdate if their dependencies are met.

flushUpdate then updates the dependencies of that stream.

The problem is flushUpdate only works for streams updated with updateStreamValue. The dependent streams function is never called in flushUpdate.

This would need to happen inside flushUpdate for those streams.

if (s.depsChanged) s.fnArgs[s.fnArgs.length - 1] = s.depsChanged;
var returnVal = s.fn.apply(s.fn, s.fnArgs);
if (returnVal !== undefined) {
  s(returnVal);
}

I think the solution lies in changing toUpdate from List Stream to List Function
flushUpdate then calls each function in the array in sequence

updateStream would then in stead of calling toUpdate.push(s)

toUpdate.push(()=> updateStream(s));

and updateStreamValue would do

toUpdate.push(()=> updateStreamValue(s, n));

This will result in

stream(1).map(function() {
  var n = flyd.stream(1);
  n.map(function(v) { result = v + 100; });
  result // undefined
});
result // 101

Meaning the mapped stream is guaranteed to run, but maybe not when you expect it to. Which should be fine since you shouldn't really be interacting with dependent streams synchronously.

[coveralls]

Coverage remained the same at 100.0% when pulling bdeb987 on nordfjord:dependent-stream-fix into 5261e97 on paldepind:master.

[coveralls]

Coverage remained the same at 100.0% when pulling eafaf79 on nordfjord:dependent-stream-fix into 5261e97 on paldepind:master.

[mrdziuban]

What's the status of this? I ran into this recently and spent a good amount of time debugging before finding #160.

[nordfjord]

@mrdziuban In my view this PR is ready to be merged. I'm sure @paldepind will look at it when he has time.

But please take note this will not fix the problem as described in #160

const result = []
stream(1).map(function() {
  var n = stream(1);
  n.map(function(v) { result.push(v + 100); });
  // 160 would like result to be already updated here
  assert(result).deepEquals([]);
});
// this PR would only guarantee that the map has run at this point
assert(result).deepEquals([101]);

The api will not be the same within and outside stream bodies. But this guarantees the map will run at some point, which it currently does not.

[paldepind]

I think this is indeed the right solution @nordfjord. Your approach to handling the two "types" of streams differently in flushUpdate definitely seems to get to the root of the issue.

As you say this still means that the streams will not have a value if you try to read it synchronously. But that seems necessary to preserve atomic updates. Otherwise, the newly created stream would have "occurrences" that triggered before reactions to the original event that lead to the creation of the stream in the first place.

Great PR 👍 Thanks a lot for fixing this issue.