Update bufferToggle closingSelector to accept promise / empty #1494
Conversation
kwonoj
force-pushed
the
fix-buffertoggle
branch
from
959990d
to
6951ac1
Compare
| @@ -23,6 +23,34 @@ describe('Observable.prototype.publishReplay', () => { | |||
| expect(source instanceof Rx.ConnectableObservable).toBe(true); | |||
| }); | |||
|
|
|||
| it('should follow the RxJS 4 behavior and NOT allow you to reconnect by subscribing again', (done: DoneSignature) => { | |||
There was a problem hiding this comment.
simple test case reordering again, due to jasmine behavior. It seems occuring repeatedly now.
| @@ -341,4 +342,54 @@ describe('Observable.prototype.bufferToggle', () => { | |||
| expectSubscriptions(e1.subscriptions).toBe(e1subs); | |||
| expectSubscriptions(e2.subscriptions).toBe(e2subs); | |||
| }); | |||
|
|
|||
| it('should accept closing selector returns promise resolves', (done: DoneSignature) => { | |||
There was a problem hiding this comment.
I would add the word that here. "should accept closing selector that returns a resolved promise"
|
Can we merge this after #1485, and then update this PR to document the use of Promises? |
kwonoj
force-pushed
the
fix-buffertoggle
branch
2 times, most recently
from
3f5a0fe
to
e6a9d39
Compare
| @@ -44,18 +46,18 @@ import {errorObject} from '../util/errorObject'; | |||
| * @owner Observable | |||
| */ | |||
| export function bufferToggle<T, O>(openings: Observable<O>, | |||
There was a problem hiding this comment.
I'm not sure, but can't openings also be a Promise?
There was a problem hiding this comment.
I'm not sure either, so limited changes for sure thing first.
There was a problem hiding this comment.
Yes, anywhere we're accepting an Observable we should accept any of the acceptable types. Although synchronous things like iterators and Arrays don't make sense here. I think we should probably accept Promise though.
Basically, we need to accept interfaces of Promise, Subscribable and IObservable here. (We probably also need better names for those last two, haha)
There was a problem hiding this comment.
OK, will try to update. Would it be ok to accept SubscribableOrPromise as same as closing?
kwonoj
force-pushed
the
fix-buffertoggle
branch
from
e6a9d39
to
0bef1ed
Compare
kwonoj
force-pushed
the
fix-buffertoggle
branch
3 times, most recently
from
ecd99ed
to
dd461e4
Compare
|
I think change's are ok, but will leave bit more to see any other suggestions around before check this in. |
kwonoj
force-pushed
the
fix-buffertoggle
branch
from
dd461e4
to
05ffaef
Compare
| @@ -44,18 +46,18 @@ import {errorObject} from '../util/errorObject'; | |||
| * @owner Observable | |||
| */ | |||
There was a problem hiding this comment.
We need to update docs in line 22 to say "an Observable or Promise returned by...".
And update docs in line 40 too.
kwonoj
force-pushed
the
fix-buffertoggle
branch
from
05ffaef
to
5838679
Compare
|
Looks like this could use a rebase. |
|
Absolutely, and this PR still need to be updated per suggestion - I'll update it soon. |
|
PR's updated per suggestion also rebased. I'll leave this PR around day before check in to see any other suggestions around. |
|
Merged with 3d22c7a. |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description:
This PR updates behavior of bufferToggle's
closingSelectorRelated issue (if exists):
relates to #1487, #1246