feat(groupBy): Adds subjectSelector argument to groupBy

[trxcllnt]

Adds a subjectSelector argument to groupBy, allowing consumers to customize the messaging semantics of the inner GroupedObservables.

Sometimes it isn't possible to immediately subscribe to groupBy's inner GroupedObservables as they're emitted, and you need to replay some number of events between when the GroupedObservable was created and when you subscribe.

For example, one encounters this problem writing multitouch gestures. Each Gesture has a set of constraints to consider the gesture started, but then needs access to the historic events once those constraints have been satisfied.

[coveralls]

Coverage increased (+0.0009%) to 97.04% when pulling 359ec8a on trxcllnt:groupby-subject-selector into 1bee98a on ReactiveX:master.

[benlesh]

This appears to be a non-breaking change.

I think I can see the utility of this, but would you might adding some use cases in a comment on this PR so everyone can better understand this addition?

Also, we'll need to update the documentation. Ideally with an additional example showing using this argument for something useful, I guess.

cc/ @staltz @mattpodwysocki what are your thoughts on this addition?

[mattpodwysocki]

I'd rename this to subjectFactory since it's not taking any input into this function

[staltz]

Agreed

[trxcllnt]

Agreed

[mattpodwysocki]

@Blesh it's an interesting choice here, although subjectSelector should be subjectFactory as it's not taking any input and always returning some sort of Subject. The only thing about this that intrigues me is changing it probably from a normal Subject to ReplaySubject in that it persists data instead of a regular Subject which is lossy.

@trxcllnt can you give us a concrete use case with working code as to why this is a good change?

[paulpdaniels]

Would it make sense to also add this for the window operators as well? There are certain cases where this sort of stuff happens if you apply various flattening techniques after windowing.

[mattpodwysocki]

@paulpdaniels yes, it's a slippery slope here once we allow you to specify the guts as it were of a given Observable of Observables.

[benlesh]

[staltz]

The only thing I don't like about this is that it makes groupBy a method with 5 arguments. But otherwise I'm favorable and can see its utility, albeit rare.

[trxcllnt]

@mattpodwysocki I admit, the use-cases for this are relatively rare, which is why I like the subjectFactory in the last arg position.

The problem that spurred me to add this option was around multitouch gesture recognition:

Observable
  .fromEvent(domElement, "touchstart")
  .groupBy(getIdentifier)
  .take(2)
  .map((starts) => starts
    .mergeMap(start => Observable
      .fromEvent(window, "touchmove")
      .filter(isIdentifier(start.identifier))
      .startWith(start)
    )
  )
  .combineAll()

By the time the second touchstart event fires, the first inner group has already fired, so the first Observable in the combineAll never emits.

[benlesh]

Thinking more on this... I feel like adding this leaks out the underlying abstraction. People don't need to know we use a Subject under the hood. I say this because it's possible that at some later point we decide to implement a groupBy that doesn't use a Subject. Perhaps for performance reasons. If we add this argument we can't really do that because we'll be stuck with using Subjects... or at least we can't do that without some crazy conditionals.

[trxcllnt]

@Blesh I see what you're saying, but since GroupedObservables are hot and multicast to multiple subscribers, how else could they be implemented? It seems any performance improvements that could be realized from a specialized "GroupedObservableSubject" probably can and should be applied to Subjects generally.

One description of groupBy is that it transforms an Observable of values into an Observable of inner hot Observables. From that angle, it makes sense to allow some sort of control over the observer-bility of inner groups.

The only other solution I've found to this problem is adding autoConnect to ConnectableObservable, which would immediately connect the ConnectableObservable to the GroupedObservable. I'm less inclined to go that route because, a. I'm certain that once autoConnect goes in, it's going to get a lot of beginners into the sort of trouble they aren't equipped to handle, and b. the double-multicasting offends my aesthetic sensibilities.

Do you have an alternate solution to the problem of missing events from GroupedObservables, or is the autoConnect direction more what you're thinking?

[coveralls]

Coverage increased (+0.0009%) to 97.041% when pulling ebd88f1 on trxcllnt:groupby-subject-selector into 8ebee66 on ReactiveX:master.

[coveralls]

Coverage increased (+0.0009%) to 97.041% when pulling 327b8d6 on trxcllnt:groupby-subject-selector into ece1b89 on ReactiveX:master.

[coveralls]

Coverage increased (+0.0009%) to 96.981% when pulling 4106d51 on trxcllnt:groupby-subject-selector into 44fbc14 on ReactiveX:master.

[jayphelps]

We decided to ship this at the meeting today.

[lock]

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.