Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(buffer): closingNotifier should support ObservableInput #7073

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions spec-dtslint/operators/buffer-spec.ts
Expand Up @@ -9,3 +9,7 @@ it('should enforce types', () => {
const o = of(1, 2, 3).pipe(buffer()); // $ExpectError
const p = of(1, 2, 3).pipe(buffer(6)); // $ExpectError
});

it('should support Promises', () => {
const o = of(1, 2, 3).pipe(buffer(Promise.resolve('foo'))); // $ExpectType Observable<number[]>
});
39 changes: 38 additions & 1 deletion spec/operators/buffer-spec.ts
@@ -1,5 +1,5 @@
import { buffer, mergeMap, take, window, toArray } from 'rxjs/operators';
import { EMPTY, NEVER, throwError, of, Subject } from 'rxjs';
import { EMPTY, NEVER, throwError, of, Subject, interval } from 'rxjs';
import { TestScheduler } from 'rxjs/testing';
import { observableMatcher } from '../helpers/observableMatcher';
import { expect } from 'chai';
Expand Down Expand Up @@ -324,6 +324,43 @@ describe('Observable.prototype.buffer', () => {
expect(results).to.deep.equal([[1], [2], [], 'complete']);
});

it('should buffer when Promise resolves', (done) => {
const e1 = interval(3).pipe(take(5));
const expected = [
[0, 1],
[2, 3, 4],
];

e1.pipe(buffer(new Promise<void>((resolve) => setTimeout(() => resolve(), 8)))).subscribe({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: (Can be fixed in a FLUP)... we don't need to wait 8 ms... we can just do Promise.resolve().

next: (x) => {
expect(x).to.deep.equal(expected.shift());
},
error: () => done(new Error('should not be called')),
complete: () => {
expect(expected.length).to.equal(0);
done();
},
});
});

it('should raise error when Promise rejects', (done) => {
const e1 = interval(1).pipe(take(5));
const error = new Error('err');

e1.pipe(buffer(Promise.reject(error))).subscribe({
next: () => {
done(new Error('should not be called'));
},
error: (err: any) => {
expect(err).to.be.an('error');
done();
},
complete: () => {
done(new Error('should not be called'));
},
});
});

describe('equivalence with the window operator', () => {
const cases = [
{
Expand Down
13 changes: 7 additions & 6 deletions src/internal/operators/buffer.ts
@@ -1,8 +1,8 @@
import { Observable } from '../Observable';
import { OperatorFunction } from '../types';
import { OperatorFunction, ObservableInput } from '../types';
import { operate } from '../util/lift';
import { noop } from '../util/noop';
import { createOperatorSubscriber } from './OperatorSubscriber';
import { innerFrom } from '../observable/innerFrom';

/**
* Buffers the source Observable values until `closingNotifier` emits.
Expand All @@ -13,7 +13,8 @@ import { createOperatorSubscriber } from './OperatorSubscriber';
* ![](buffer.png)
*
* Buffers the incoming Observable values until the given `closingNotifier`
* Observable emits a value, at which point it emits the buffer on the output
* `ObservableInput` (that internally gets converted to an Observable)
* emits a value, at which point it emits the buffer on the output
* Observable and starts a new buffer internally, awaiting the next time
* `closingNotifier` emits.
*
Expand All @@ -36,12 +37,12 @@ import { createOperatorSubscriber } from './OperatorSubscriber';
* @see {@link bufferWhen}
* @see {@link window}
*
* @param {Observable<any>} closingNotifier An Observable that signals the
* @param closingNotifier An `ObservableInput` that signals the
* buffer to be emitted on the output Observable.
* @return A function that returns an Observable of buffers, which are arrays
* of values.
*/
export function buffer<T>(closingNotifier: Observable<any>): OperatorFunction<T, T[]> {
export function buffer<T>(closingNotifier: ObservableInput<any>): OperatorFunction<T, T[]> {
return operate((source, subscriber) => {
// The current buffered values.
let currentBuffer: T[] = [];
Expand All @@ -59,7 +60,7 @@ export function buffer<T>(closingNotifier: Observable<any>): OperatorFunction<T,
);

// Subscribe to the closing notifier.
closingNotifier.subscribe(
innerFrom(closingNotifier).subscribe(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benlesh, in the RxJS Core Semantics guide, it is stated that:

"Notifiers" provided directly to the operator MUST be subscribed to before the source is subscribed to.

This is not the case with this operator (and it's unrelated to this PR as well, but I noticed this here) - the source gets subscribed to on line 51. Should we address this? If yes, should we do it in V7 or should we wait for V8?

createOperatorSubscriber(
subscriber,
() => {
Expand Down