Rxjs: buffer drops last buffer on source complete (again)

[t1bb4r]

Bug Report

Current Behavior
buffer drops last buffer on source complete.

[0, 1, 2, 3, 4, 5, 6, 7, 8]

Reproduction
https://stackblitz.com/edit/typescript-4rc7ej

const tick = interval(100).pipe(take(15))

tick.pipe(buffer(interval(1000)))
  .subscribe((x) => console.log(x))

Expected behavior

[0, 1, 2, 3, 4, 5, 6, 7, 8]
[9, 10, 11, 12, 13, 14]

Environment

• Runtime: Node v8.11.2
• RxJS version: 6.2.2

Possible Solution
The fix is almost the same as in #2174, due to breaking changes I just translated it to be compatible with rxjs6, https://github.com/t1bb4r/rxjs/commit/ed3c82d4521a24990f0699733ec98739a4bfce53

Additional information
This bug has already been reported and fixed at some point, but it looks to me like the commit got lost between rxjs 5 and 6, because it's not on either of the two now. (again see #2174). I am now reporting that it is not working in rxjs 5 or 6.2.2 and I couldn't find any issues or pull requests removing this functionality.

[cartant]

It's unfortunate that #2174 wasn't merged into v6, but I guess the reasoning referenced in this comment still applies:

Looks good, but even though we didn't intend for the existing behavior, since we shipped it people could be relying on it.

Merge into next major release, not minor because it is a BREAKING CHANGE.

[t1bb4r]

I'm still a bit confused (sorry for ignorance). So it was merged, but into the Reactivex:next branch, which I thought was for v6. Does this mean it's part of the next major release (v7)? I also can't see the next branch anymore. It seriously looks to me like this commit just got lost some how, it's really old (31st Jan 2017).

It was tagged as 6.0.0-alpha.0 and even added to the list of bug fixes: https://github.com/ReactiveX/rxjs/commit/0b2ddf2d2c0bac0943a2219d8852b59461295b31

[cartant]

When v6 was released, master was renamed to stable and next became the master. There is no current next branch; work for v7 is in experimental.

I've done some investigating. The PR you referenced was merged into v6 (next). You can see the source for the release you mentioned here:

https://github.com/ReactiveX/rxjs/blob/6.0.0-alpha.0/src/operator/buffer.ts

And you can see the history for that file here:

https://github.com/ReactiveX/rxjs/commits/6.0.0-alpha.0/src/operator/buffer.ts

If you look at the source for the alpha release that follows the one you referenced, it seems that the commit and the one that followed in the above history were lost in the directory restructuring:

https://github.com/ReactiveX/rxjs/blob/6.0.0-alpha.1/src/internal/operators/buffer.ts

So I guess that none of the v6 releases that followed the first alpha have included either of those fixes.

[cartant]

Given that both #2174 and #2175 were breaking changes, I guess - for the same reasoning they weren't merged in v5 - they'll need to go into v7, now.

/cc @benlesh

[t1bb4r]

Aha, makes sense now. So it was lost in the directory restructuring. Well that's ok, it happens. Do we agree that this is still an issue and it will be addressed in the next release? For me losing "events" is a major issue so I'd really like this fix.

Thanks for all the help thus far. I was really confused.

[cartant]

If the commits got 'lost', I think they need to be reinstated - for v7 - as they'd already been deemed fixes.

AFAICT, there was no reason for their being reverted. I'll have a closer look, later, at the commits that occurred between the two alpha releases - to see if I can better understand what happened.

[benlesh]

What we'll need is the tests from those commits, perhaps add them into the current test suite, but commented out with // FOR v7 or something like that? At some point, all of the tests need to be ported over to the experimental branch

[benlesh]

This is now waiting in the v7 branch

[t1bb4r]

In 61b1767, it doesn't look like any of the tests cover the following case:

  it('should emit last buffer if source completes', () => {
    const a =    hot('-a-b-c-d-e-f-g-h-i-|');
    const b =    hot('-----B-----B--------');
    const expected = '-----x-----y-------(z|)';
    const expectedValues = {
      x: ['a', 'b', 'c'],
      y: ['d', 'e', 'f'],
      z: ['g', 'h', 'i']
    };
    expectObservable(a.pipe(buffer(b))).toBe(expected, expectedValues);
  });