Fix/3990 buffer emit remainder on complete

[benlesh]

fix(buffer): Remaining buffer will be emited on source close if closingNotifier active

Gives the author control over the emission of the final buffer. If the closingNotifier completes before the source does, no more buffers will be emitted. If the closingNotifier is still active when the source completes, then whatever is in the buffer at the time will be emitted from the resulting observable before it completes.

BREAKING CHANGE: Final buffered values will now be emitted from the resulting observable if the closingNotifier is still active. The simplest workaround if you want the original behavior (where you possibly miss values), is to add a skipLast(1) at the end. Otherwise, you can try to complete the closingNotifier prior to the completion of the source.

Fixes #3990, #6035

fix(buffer): closingNotifier completion does not complete resulting observable

Resolves an issue where the resulting observable would complete when the closingNotifier completed. Notifier completion should not complete the result, only source completion should do that.

BREAKING CHANGE: closingNotifier no longer closes the result of buffer. If that is truly a desired behavior, then you should use takeUntil. Something like: source$.pipe(buffer(notifier$), takeUntil(notifier$.pipe(ignoreElements(), endWith(true)))), where notifier$ is multicast, although there are many ways to compose this behavior.

[benlesh]

What you're going to see through a lot of these tests is I had to add the last emission of an empty buffer for cases when the closingNotifier was not complete when the source completed.

[cartant]

Yeah, but TBH I am a little disappointed that you didn't use an emoji:

      const a = hot('   -a-b-c-d-e-f-g-h-i-|');
      const b = hot('   -----B-----B-----B-|');
      const expected = '-----x-----y-----z-(🦖|)';
      const expectedValues = {
        x: ['a', 'b', 'c'],
        y: ['d', 'e', 'f'],
        z: ['g', 'h', 'i'],
        '🦖': [],
      };

[cartant]

LGTM. I'm going to push another commit to this branch to add some tests that assert/document what I recall you mentioning should be an equivalence between buffer and window (with toArray applied). However, one of the cases in the tests fails. If this equivalence is expected, perhaps we should looks at what's different? I don't have time to do this, right now - I can look at it tomorrow. Feel free to toss the commit if it's misguided or or whatever. I won't be offended. 🙂

[benlesh]

@cartant it looks like we caught another inconsistency with our API. The window operator is allowing the completion of the windowBoundaries notifier to act as a notification, rather than a simple completion. It's completing the result and the window, when really it should just mean that the author is saying they're not going to close and reopen any new windows again.

(It's a bug of the same type, basically)

[benlesh]

@cartant So I've added another commit that also fixes window to behave properly, and changes the behavior of buffer to the simpler thing that makes more sense. They are now equivalent. I've also rebased the commits to make sure we don't have conflicting messages in our changelog and added examples on how to get the "old" behavior to the breaking change message.

[cartant]

LGTM, just some nitpicks.

[benlesh]

Merging despite ts@latest issue that seems unrelated.