Support optional selector for publish / multicast

[kwonoj]

Description:
This PR updates behavior of multicast and publish as well, support optional selector functions can be supplied.

Related issue (if exists):
#1629

_Note_
As I'm not frequent user of publish/multicast, have gut feeling this PR might be conceptually incorrect at all. Please feel freely point out / or close PR if this isn't correct approach.

[benlesh]

@kwonoj what were the perf changes for this addition?

[kwonoj]

I haven't observed noticeable regression between this changes to master, but current master itself shows some interesting numbers on my machine

master, without this PR

                      |         RxJS 4.1.0 | RxJS 5.0.0-beta.6 | factor | % improved
-------------------------------------------------------------------------------------
multicast - immediate | 9,526,302 (±3.72%) |2,132,759 (±4.14%) |  0.22x |     -77.6%

may possible

• weird on my machine only (as always)
• perf test design is incorrect

I've added couple of commits for micro perf test as well.

[benlesh]

@kwonoj it's probably the new implementation. In Rx4, multicast is highly polymorphic. What I mean by that is it has two wildly different code paths depending on the parameters provided.

I haven't dug into your implementation yet, but this was one of the primary reasons this functionality wasn't reintroduced immediately.

[benlesh]

This seems like a smell. Why do we need to do this?

[kwonoj]

To support different signature multicast(subject) / multicast(factory, selector?).

As similar to other operators have different signatures mergeMapTo / add/mergeMapTo if there's completely different two signatures, can't be assigned into prototype directly since function definition is single, polymorphic.

[benlesh]

I've added a few comments, and pointed out one area I can see there might be a perf bottleneck.

[kwonoj]

Updated PR per suggestions.

One thing about static create for multicast is not included, after give a couple of try I found it's quite challenging to create static signature due to overloaded signatures of multicase is completely different and incompatible which doesn't allow simple method signature overloading in TypeScript. May need some more exploration to see if I can bring it, will followup with separate PR once I can make it work.

[benlesh]

due to overloaded signatures of multicase is completely different and incompatible which doesn't allow simple method signature overloading in TypeScript.

Yeah, we ran into this with Subject.create as well. See that for a workaround. Also there's an issue I filed about it: microsoft/TypeScript#4628

[kwonoj]

I've tried to apply similar practices as Subject.create, seems I did something not correctly.

Basically what need to be done is (in pseudo)

class Observable {
 static multicast(subjectOrSubjectFactory: Subject<T> | (() => Subject<T>))
        : ConnectableObservable<T>;
 static multicast(SubjectFactory: () => Subject<T>,
                 selector?: (source: Observable<T>) => Observable<T>): Observable<T> {
 }
}

function signature overloading in Observable without inheritance being applied. workaround for Observable.create seems doesn't allow to create method overload signatures - is there way for this?

/cc @david-driscoll also for visibility / tips.

[benlesh]

@kwonoj how is perf after the change?

[benlesh]

Where is @jayphelps with his 5th element "multicast" meme?

[jayphelps]

Sorry I'm late!

[kwonoj]

Numbers with current changes are below.

                                |         RxJS 4.1.0 |   RxJS 5.0.0-beta.6 |    factor |   % improved
------------------------------------------------------------------------------------------------------
 multicast-selector - immediate |    32,802 (±3.46%) |     59,618 (±3.62%) |     1.82x |        81.7%


                                |         RxJS 4.1.0 |   RxJS 5.0.0-beta.6 |    factor |   % improved
------------------------------------------------------------------------------------------------------
          multicast - immediate | 8,695,218 (±3.18%) |  2,351,401 (±2.43%) |     0.27x |       -73.0%

muitlcast itself doesn't seem regresses.

[benlesh]

@kwonoj I think we can merge this as-is and circle back to perf. I have a few ideas.

[benlesh]

Merged with 0f8bf28. Thanks @kwonoj

[staltz]

By the way, I know it's kind of late, but I just remembered that this should also be done for publishReplay and publishBehavior, since they exist in both RxJS v4 and RxJava.

[trxcllnt]

@kwonoj shouldn't this really be done with an operator? Why introduce another Observable type?

[kwonoj]

I didn't have strong reasoning behind this, actually. So suggest to remove this observable type?

[trxcllnt]

Well, it'll make the ConnectableObservable changes I'm working on easier to integrate.

[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.