staltz / xstream

An extremely intuitive, small, and fast functional reactive stream library for JavaScript
http://staltz.github.io/xstream/
MIT License
2.37k stars 137 forks source link

fix(dropUntil): align with rxjs skipUntil #258

Closed xtianjohns closed 6 years ago

xtianjohns commented 6 years ago

Refactor dropUntil to leave its output stream unchanged when its 'other' stream completes. Make output stream empty if 'other' stream is empty.

This change aligns dropUntil more closely with the skipUntil operator from rxjs.

Description

The other internal listener in dropUntil now treats completion as a noop. Additionally, when source completes, we no longer call up(), which seems like a pretty major bug to begin with.

Instead, completion of the source now calls _stop(), which removes the internal listener to source and detaches the output stream from the operator. This is definitely a situation where we need ComVer, since consumers may have unknowingly relied on this behavior, which never removed the internal listener to the source stream.

Resolves #237.

Motivation and Context

dropUntil is designed to keep a result stream silent until some other stream emits. However, the implementation of dropUntil left ambiguous how the operator should behave when either the source stream, or the other (notifier) stream, completes.

The documentation for dropUntil gave consumers the impression that the result stream would complete if the 'other' stream completed. However, we had no tests that verified this behavior was correct. Confusion about how the operator should behave led to opening #237.

Aligning the operator's behavior with another implementation (from rxjs) is okay, but I consider this change to really be an improvement to the operator. Treating completion as a noop is essential because there is no semantic meaning associated with the other stream completing, for this operator. If a consumer needed semantics for declaring when their stream should end, they can reach for endWhen, or another extra.

Additionally, if the consumer wanted the semantics of dropUntil and didn't want endWhen behavior, this extra wasn't helpful. They'd have to roll their own.

How Has This Been Tested?

I've changed one existing test, and added two more. The altered test has had its expectation reversed:

should not complete the stream when another non empty stream emits complete

The new tests are largely borrowed from the rxjs expectations of skipUntil. They can be summarized as:

If source is infinite, the result stream is infinite. If source completes, the result stream completes.

Checklist:

staltz commented 6 years ago

Really impressive PR, thanks @xtianjohns. No problems found.

It'll cause a breaking change release. (note for self)