Fix groupBy() to use lift() #1128

staltz · 2016-01-04T14:46:14Z

Addresses bug #1085.

Adds tests on Observable to verify that basic lift() features work.
Adds tests on groupBy to assert it does not break lift() features.
Fixes groupBy to use lift().

BREAKING CHANGES:
groupBy() now unsubscribes all inner Observables when the outer
Observable is unsubscribed. This diverges from the behavior in RxJS 4,
where inner Observables continue even if the outer is unsubscribed.

RxJS 4 explicitly supports the "inners continue when outer is unsubscribed", see this test. @Blesh @trxcllnt @mattpodwysocki what do you think about the breaking change I'm proposing? It seems odd to me why shouldn't we dispose of inners. I don't know if and why there is anyone depending on this behavior.

mattpodwysocki · 2016-01-04T14:57:34Z

@staltz I'm not a fan of this breaking change as the inners should be independent of the outers which makes them more reliable as you pass the inners around for streaming purposes and to be weirdly affected by something upstream. It's best to be kept as it was in RxJS v4 cc @headinthebox

benlesh · 2016-01-07T23:12:35Z

@mattpodwysocki I can see what you're talking about, but I'm having a hard time coming up with example code that would do this without doing something obviously strange.

benlesh · 2016-01-07T23:14:10Z

Also, why wouldn't we terminate those groups if they couldn't possibly produce another value because the source was terminated? It sounds more like a bug than a feature, TBH.

benlesh · 2016-01-07T23:16:50Z

Perhaps a middle-ground would be to have the inner streams error when the upstream is unsubscribed with a custom error like UnsubscribedSourceError. Then if the desired behavior is for the stream to continue on, you could handle that specific error with catch and return an Observable.never().

benlesh · 2016-01-13T01:10:06Z

@staltz ... this needs to be rebased before I can merge it.

staltz · 2016-01-13T14:37:01Z

Rebased! :)

mattpodwysocki · 2016-01-13T18:15:47Z

@staltz @Blesh I still have concerns about the independence of the inner versus outer subscriptions. I wouldn't merge until we come up to some agreement on this because it diverges from all other Rx implementations.

staltz · 2016-01-13T18:24:54Z

@mattpodwysocki window also behaves like this (inners are unsubscribed when the outer is unsubscribed). Why is groupBy different? Who actually needs the use case you are portraying?

trxcllnt · 2016-01-13T22:11:09Z

@staltz GroupedObservable is similar to ConnectableObservable, since they both front for a shared Subject. Multiple people can connect to the same ConnectableObservable without interfering with each other. The same should be true for GroupedObservable.

I know that's how GroupedObservable works in this PR, but this leads me to my next point:

Once groupBy has emitted a GroupedObservable, that GroupedObservable has left the safety and comfort of the groupBy operator. Disposing the GroupBySubscriber shouldn't dispose inner GroupedObservables, because they might have subscription side-effects that groupBy doesn't know about.

For example, if you multicast a GroupedObservable with a ReplaySubject, unsubscribing from the GroupBySubscriber would call unsubscribe on the ReplaySubject, which would be incorrect.

mattpodwysocki · 2016-01-13T22:13:06Z

@trxcllnt well said, and exactly my concerns that I had, which is why inner independence was necessary.

staltz · 2016-01-14T12:29:33Z

Does that apply also to window?

RxJS 4 https://jsbin.com/pukeni/edit?js,console
RxJS 5 https://jsbin.com/sajuwu/edit?js,console

mattpodwysocki · 2016-01-14T16:07:31Z

@staltz no, this not independent of the outer subscription, upon disposal, it still ensures that the window is eventually emptied and then it stops.

staltz · 2016-01-18T13:23:33Z

@Blesh @mattpodwysocki this PR is now safe to merge, I re-implemented the solution to keep it backwards-compatible as Matt wishes. We can/could postpone the discussion on subscription independence. The priority was to get groupBy working with lift().

mattpodwysocki · 2016-01-18T18:17:14Z

@staltz To be fair, it was both @trxcllnt and myself who wanted to keep independence. Also, I don't see the tests associated with this to check for the independence of the inner from the outer such as these from groupBy and groupByUntil

staltz · 2016-01-18T18:33:35Z

Also, I don't see the tests associated with this to check for the independence of the inner from the outer such as these from

The tests were not added in this PR because they are already there, I had ported them from RxJS 4 to this repo long ago.

benlesh · 2016-01-18T19:42:26Z

LGTM.

mattpodwysocki · 2016-01-18T20:12:00Z

@staltz ah, sorry about that. LGTM then!

kwonoj · 2016-01-21T22:31:14Z

src/Observable.ts

@@ -211,7 +211,7 @@ export class Observable<T> implements CoreOperators<T> {
 projectResult?: (x: T, y: any, ix: number, iy: number) => R,
 concurrent?: number) => Observable<R>;
 flatMapTo: <R>(observable: Observable<any>, projectResult?: (x: T, y: any, ix: number, iy: number) => R, concurrent?: number) => Observable<R>;
- groupBy: <K, R>(keySelector: (value: T) => string,
+ groupBy: <T, K, R>(keySelector: (value: T) => K,


do we need to add T as function generic parameter? expect T comes from generic type of Observable<T>.

I remember TS was complaining and this solved it, but I can re-check.

staltz · 2016-01-25T09:09:31Z

@kwonoj I updated the TS typing issue that you pointed out.

kwonoj · 2016-01-25T17:16:47Z

Thanks, change looks good to me too.

benlesh · 2016-01-25T21:36:25Z

:) Sorry @staltz, can you rebase this one?

Add tests for Observable to verify that the most important codebase-wide features brought by the lift architecture are satisfied. Refers to discussion in ReactiveX#60.

Add test for groupBy() operator, to verify that it supports the composable lift architecture. Test for issue ReactiveX#1085.

Fix bug ReactiveX#1085 in which groupBy was not using lift(). Using lift() is critical to support the ubiquitous lift-based architecture in RxJS Next Resolves ReactiveX#1085.

staltz · 2016-01-26T08:13:29Z

Alright, done

benlesh · 2016-01-27T17:16:36Z

Merged as of 39ebae9... I basically took your whole PR as-is during rebase and reapplied my perf fixes.

Thanks @staltz! This was an important one.

lock · 2018-06-07T12:03:00Z

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.

benlesh added the blocked label Jan 13, 2016

staltz force-pushed the groupBy-lift branch from 06df9d9 to 67d1d3d Compare January 13, 2016 14:21

staltz force-pushed the groupBy-lift branch 2 times, most recently from 0ef4864 to f0ad32a Compare January 18, 2016 13:15

benlesh added PR: lgtm and removed blocked labels Jan 18, 2016

kwonoj reviewed Jan 21, 2016
View reviewed changes

staltz force-pushed the groupBy-lift branch from f0ad32a to 3b2f7ef Compare January 25, 2016 09:08

test(Observable): add tests for important lift() features

93126b1

Add tests for Observable to verify that the most important codebase-wide features brought by the lift architecture are satisfied. Refers to discussion in ReactiveX#60.

staltz added 2 commits January 26, 2016 10:02

test(groupBy): add test asserting lift is not broken

57ab584

Add test for groupBy() operator, to verify that it supports the composable lift architecture. Test for issue ReactiveX#1085.

fix(groupBy): fix groupBy to use lift(), supports composability

3d58543

Fix bug ReactiveX#1085 in which groupBy was not using lift(). Using lift() is critical to support the ubiquitous lift-based architecture in RxJS Next Resolves ReactiveX#1085.

staltz force-pushed the groupBy-lift branch from 3b2f7ef to 3d58543 Compare January 26, 2016 08:07

benlesh closed this Jan 27, 2016

staltz deleted the groupBy-lift branch January 27, 2016 20:13

lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix groupBy() to use lift() #1128

Fix groupBy() to use lift() #1128

staltz commented Jan 4, 2016

mattpodwysocki commented Jan 4, 2016

benlesh commented Jan 7, 2016

benlesh commented Jan 7, 2016

benlesh commented Jan 7, 2016

benlesh commented Jan 13, 2016

staltz commented Jan 13, 2016

mattpodwysocki commented Jan 13, 2016

staltz commented Jan 13, 2016

trxcllnt commented Jan 13, 2016

mattpodwysocki commented Jan 13, 2016

staltz commented Jan 14, 2016

mattpodwysocki commented Jan 14, 2016

staltz commented Jan 18, 2016

mattpodwysocki commented Jan 18, 2016

staltz commented Jan 18, 2016

benlesh commented Jan 18, 2016

mattpodwysocki commented Jan 18, 2016

kwonoj Jan 21, 2016

staltz Jan 22, 2016

staltz commented Jan 25, 2016

kwonoj commented Jan 25, 2016

benlesh commented Jan 25, 2016

staltz commented Jan 26, 2016

benlesh commented Jan 27, 2016

lock bot commented Jun 7, 2018

Fix groupBy() to use lift() #1128

Fix groupBy() to use lift() #1128

Conversation

staltz commented Jan 4, 2016

mattpodwysocki commented Jan 4, 2016

benlesh commented Jan 7, 2016

benlesh commented Jan 7, 2016

benlesh commented Jan 7, 2016

benlesh commented Jan 13, 2016

staltz commented Jan 13, 2016

mattpodwysocki commented Jan 13, 2016

staltz commented Jan 13, 2016

trxcllnt commented Jan 13, 2016

mattpodwysocki commented Jan 13, 2016

staltz commented Jan 14, 2016

mattpodwysocki commented Jan 14, 2016

staltz commented Jan 18, 2016

mattpodwysocki commented Jan 18, 2016

staltz commented Jan 18, 2016

benlesh commented Jan 18, 2016

mattpodwysocki commented Jan 18, 2016

kwonoj Jan 21, 2016

Choose a reason for hiding this comment

staltz Jan 22, 2016

Choose a reason for hiding this comment

staltz commented Jan 25, 2016

kwonoj commented Jan 25, 2016

benlesh commented Jan 25, 2016

staltz commented Jan 26, 2016

benlesh commented Jan 27, 2016

lock bot commented Jun 7, 2018