Side effects and ordering in flyd.on

[irisjae]

var x = flyd.stream()
flyd.on(console.log.bind(console,'one'),x)
flyd.on(console.log.bind(console,'two'),x)
flyd.on(console.log.bind(console,'three'),x)
x('fire')

This gives the result

three fire
two fire
one fire

This seems counter intuitive to me and caused some issues for me. Is there any rationale for this behavior? Could we change it? This seems like a major design decision, and if this was intended, I cannot understand the rationale.

[irisjae]

Changing this section in updateDeps(s)

for (; orderNextIdx >= 0; --orderNextIdx) {
    o = order[orderNextIdx];
    if (o.shouldUpdate === true) updateStream(o);
    o.queued = false;
  }

to this

  for (var x = 0; x <= orderNextIdx; x++) {
    o = order[x];
    if (o.shouldUpdate === true) updateStream(o);
    o.queued = false;
  }

seems to make it work as expected for me, though I'm not sure if there are any side consequences.

[dmitriz]

According to https://github.com/paldepind/flyd#flydonfn-s, the on method is essentially a "worse" version of map as it does the same but does not return anything useful.

And if you use map instead of on, that behaviour becomes intuitive. Mapping over a function f means f is called with every stream value. So when you update your stream, all 3 functions are called. Does it make sense?

[StreetStrider]

@dmitriz, he notes on the order of called functions, not the function itself.

[dmitriz]

@StreetStrider Aren't they meant to fire simultaneously?

[StreetStrider]

@dmitriz what do you mean by «simultaneous» here?

[dmitriz]

@StreetStrider All at the same time, as soon as their parent emits.

[StreetStrider]

@dmitriz, yes, but this is irrelevant to what TS mentions. He just notes on that on-subscribed functions are called in reverse.

[dmitriz]

@StreetStrider The order is not guaranteed by the spec. It may depend on the implementation and on the runtime environment. Why does it matter?

[irisjae]

For side effects: clearly it could have great impact

[garkin]

This is a result of topological sort of the dependency graph. It's needed for atomic updates. For example in this case sorted nodes array will look like [OnBbb, bbb, OnCc, cc, bb, b, c, d]. Array with sort results should always be iterated in reverse to the build order due the nature of such sort. This is why you could not change iteration order in the unwrapping loop.

But it is possible to reverse node children iteration order during build. (both for(of [...listeners]) loops in the updateDeps() and in the findDeps()) In this case you will get following order:

And sorted nodes: [d, OnCc, cc, c, OnBb, bbb, bb, b]. This will result in the sort being sometimes closer to the natural order of children.

[StreetStrider]

Wow, nice picture.

[garkin]

The problem is a backward compatibilty. It will change the order of execution for already existing programs. It would be a breaking change.

If we forget about that - it could be usefull for simple cases. Could be planned for a major version increment.

On the second thought, in complex and dynamic topologies it won't help. Order there won't be obvious from the declaration, and you should not rely on it.

[nordfjord]

This is a quirk of the library as is. Looking at the commit history the reverse iteration is because of a rather nice performance boost.

If you want some b to happen after some a you need to make it explicit by making b depend on a e.g. for your original example.

const log = a => x => (console.log(a, x), x);
const x = stream();
x.map(log('first')).map(log('second')).map(log('third'));
x('fire');

Now in stead of the dependency graph looking like this:

   x
 / | \
1  2  3

It looks like

x
|
1
|
2
|
3

Meaning things will happen in your preferred order.

I'm not sure this is something we'd like to change, given it's here for perf reasons.