combine() exposes inner state

[svachalek]

I noticed this when adding dropRepeats to the output of a combine; the stream stopped producing anything.

What appears to be happening is that the combine operator is producing an array, mutating it, and sending the same array again, so dropRepeats ends up comparing the array to itself, in its current state, and it always comes up equal.

It can be worked around by putting map(a => a.slice(0)) in between, but it seems to me this [passing the inner state to next()] is a dangerous practice since user code could end up naively mutating the array and causing subtle bugs.

[staltz]

Yes I'm aware of this behavior, but before doing these changes to combine, can I know why are you using dropRepeats after combine? (It usually means you are looking for something like #102). The common use case for combine is to use a map just after it. We get performance benefits by not slicing the array. We could slice it internally in combine just to make combine more correct, but only if it supports recommended use cases.

[svachalek]

The common case for me as well is to just map it into an object, but in this case it was essentially:

xs.combine(a$, b$, c$, $d).map(([a, b, c, d]) => /* HTTP call */).flatten()

This shouldn't come up in CycleJS but this is in a hybrid context so things are not quite as pure. I could dropRepeats on each input but it was less messy code-wise to stick a dropRepeats before the map.

It's definitely a corner case, maybe more worthy of an FYI in the doc rather than a code change. But I thought I'd bring it up just on the chance someone would walk away with the array reference and wind up with some really confusing behavior. There's probably some way to create an immutable proxy for an array that would prevent that too, although it would still trick the dropRepeats filter.

[Hypnosphi]

Is it possible to dropRepeats on resulting request object in your case?

[staltz]

I'm thinking that we could attempt to make this fix in dropRepeats instead of in combine.

[Hypnosphi]

How exactly? By performing deep comparison each time?

[staltz]

Not by doing deep comparison inside dropRepeats, but deep comparison in the isEqual argument of dropRepeats. While the fix inside dropRepeats would be check if every incoming value is an array, and if true, then slice it before keeping in memory. So I'm suggesting the fix to be both in user code and library code. We have to also fix it on user code because of the following:

dropRepeats uses triple equality by default, which may mean equality by value (strings, numbers) or equality by reference (objects, arrays). You should avoid comparing by reference unless you actually want it. 'foo' === 'foo' is always true because it's equality by value (or contents if you will), while {} === {} is always false because it's equality by reference and these are references to two different newly created objects. So xs.of({}, {}, {}, {}).compose(dropRepeats()) will emit {}, {}, {}, {}, and that's not a bug in xstream, because dropRepeats() is dropRepeats(tripleEquals).

So because the combine stream is a stream of arrays, we shouldn't use tripleEquals, because even if we add a slice inside combine, if the outcome is a stream of [1,2,3], [1,2,3], [1,2,3] these are 3 different references to arrays, so dropRepeats would emit [1,2,3], [1,2,3], [1,2,3].

That's why it's better to do this fix using dropRepeats argument, with deep comparison or hashing, assuming that the internal memory of previous emissions in dropRepeats is not mutated:

streamOfArrays.compose(dropRepeats(deepEqual))

streamOfArrays.compose(dropRepeats((x, y) => hashOf(x) === hashOf(y)))

[staltz]

Fixed in v5.3.6