Provide a way to unsubscribe streams

[charlag]

Description

The most common pattern for reactive programming is mapping/chaining streams. Unfortunately, current implementation of unsubscribing is not intuitive and creates small memory leaks.

Currently ending stream means "this stream is ended and it doesn't have any dependent streams anymore". What is surprising for me (and for other people) is that it doesn't mean "I let my parent stream know that I'm not interested anymore". In practice it leads to such code:

let value = editor.textfield.value.map(address => {
	if (address.trim().length > 0) {
                doSomething()
		value.end(true)
	}
})

The person who wrote that (reasonably) expected, that the callback of "map" won't be executed, because it should be associated with the dependent stream. In reality callback is associated with the parent stream and the parent stream is not checking if dependent stream is closed or not before calling the callback:

// mithril/stream.js

function stream(v) {
	if (arguments.length && v !== Stream.SKIP && open(stream)) {
		value = v
		stream.changing()
		stream.state = "active"
		dependentStreams.forEach(function(s, i) { s(dependentFns[i](value)) })
	}

	return value
}

Current workaround looks like this:

let value = editor.textfield.value.map(identity)
value.map(address => {
	if (address.trim().length > 0) {
                doSomething()
		value.end(true)
	}
})

Note that identity function will still be called forever.

Possible Implementation & Open Questions

1. Check if stream has ended before calling dependent function

dependentStreams.forEach(function(s, i) { open(s) && s(dependentFns[i](value)) })

2. 1 + Remove dependent function to avoid memory leaks
3. Re-architecture to have mapping function inside the dependent stream
4. Re-architecture to have a refence to the parent stream and be able to tell it to remove us from the dependent list

Is this something you're interested in working on?

I can try to hack it together if we agree on approach.

Please note that this is a breaking change.

[dead-claudia]

I'm not convinced this has to be a breaking change, BTW - you could make it purely additive, and I'd prefer a non-breaking solution better.

[charlag]

Indeed, we can add some other way to end stream with dependent functions, either with another argument to end() or with another function. API could become more confusing but this is the price for compatibility I guess. I will take anything.

[dead-claudia]

@charlag Now that I take a closer look at this, it looks more like a design oversight.

• The docs for stream.end state "A co-dependent stream that unregisters dependent streams when set to true. See ended state."
• The docs for "ended state" state that stream.end(true) "effectively removes the connection between a stream and its dependent streams."
• Both of these miss the case of unregistering a stream from its parent.

So in reality, doing 1 and 2, not emitting to ended streams and deregistering them from their parent on .end(true), would be fixing an oversight. It's technically breaking, but I don't believe anyone would reasonably expect it to still be called from reading the docs.

---

I did notice another bug: the docs for "ended state" state "Ended streams still have state container semantics, i.e. you can still use them as getter-setters, even after they are ended", but the value isn't actually assigned unless the stream is opened

[charlag]

@isiahmeadows thanks for looking at it!
That's why I propose to do it before the next release - while it is undocumented behavior existing code relies on it and there's no guarantee that the version bump will keep behavior the same.

Should we discuss possible solutions or you would prefer to design it yourself?

[dead-claudia]

@charlag It's just as simple as doing these three things:

• Adding a reference to the parent for dependent streams
• Removing the stream from the parent when the stream is ended
• And for the side bug, lifting the open(stream) guard to cover only these statements.

It's worth noting that this used to be the behavior, and it was accidentally broken in #2207. So although fixing this is non-breaking, the break is reverting a previous breaking change that resulted in 0 bug reports until now. 😉

There's a couple other issues, too.

[charlag]

@isiahmeadows oh, neat, then it got broken when we updated Mithril 😅

I'm okay with adding a reference to the parent. While it's a cycling dependency, there should be no engines confused by that.
I'll try to implement it next week. Thanks a lot for being responsive!

[dead-claudia]

@charlag I'm not going to require you to spot the other issues. A PR with the three bits listed above and a fix to correctly propagate stream ending should be sufficient. If you need any help while writing it, let me know. 🙂

[dead-claudia]

Fixed by #2369