publishBehavior and publishReplay semantics when completed

staltz commented 9 years ago

Continuing the discussion that started here: https://github.com/ReactiveX/RxJS/pull/235/files

RxJS Legacy (v4) passes these tests:

var results1 = [];
var results2 = [];
var subscriptions = 0;

var source = Rx.Observable.create(function (observer) {
  subscriptions++;
  observer.onNext(1);
  observer.onNext(2);
  observer.onNext(3);
  observer.onNext(4);
  observer.onCompleted();
});

var hot = source.shareValue(0);

hot.subscribe(x => results1.push(x));

expect(results1).toBe([0,1,2,3,4]);
expect(results2).toBe([]);

hot.subscribe(x => results2.push(x));

expect(results1).toBe([0,1,2,3,4]);
expect(results2).toBe([]);
expect(subscriptions).toBe(2);

and

var results1 = [];
var results2 = [];
var subscriptions = 0;

var source = Rx.Observable.create(function (observer) {
  subscriptions++;
  observer.onNext(1);
  observer.onNext(2);
  observer.onNext(3);
  observer.onNext(4);
  observer.onCompleted();
});

var hot = source.shareReplay(2);

hot.subscribe(x => results1.push(x));

expect(results1).toBe([1,2,3,4]);
expect(results2).toBe([]);

hot.subscribe(x => results2.push(x));

expect(results1).toBe([1,2,3,4]);
expect(results2).toBe([3,4]);
expect(subscriptions).toBe(2);

Yet RxJS Next behaves basically the opposite way:

Failures:
1) Observable.prototype.publishBehavior() should not emit next events to observer after completed
  Message:
    Expected [ 0 ] to equal [  ].
  Stack:
    Error: Expected [ 0 ] to equal [  ].
      at Object.<anonymous> (./RxJSNext/spec/operators/publishBehavior-spec.js:65:21)
2) Observable.prototype.publishReplay() should emit replayed events to observer after completed
  Message:
    Expected [  ] to equal [ 3, 4 ].
  Stack:
    Error: Expected [  ] to equal [ 3, 4 ].
      at Object.<anonymous> (./RxJSNext/spec/operators/publishReplay-spec.js:98:21)
145 specs, 2 failures
Finished in 12.951 seconds

Because in RxJS legacy there is

    ConnectableObservable.prototype._subscribe = function (o) {
      return this._subject.subscribe(o);
    };

While in RxJS Next:

class ConnectableObservable<T> extends Observable<T> {
  subject: Subject<T>;

  // ...

  _subscribe(subscriber) {
    return this._getSubject().subscribe(subscriber);
  }

  _getSubject() {
    const subject = this.subject;
    if (subject && !subject.isUnsubscribed) {
      return subject;
    }
    // Subject will be recreated
    return (this.subject = this.subjectFactory());
  }

  // ...
}

In my opinion, the new behavior in RxJS Next is illogical at first sight, and a potential source for a lot of developer confusion. Besides that, it differs from RxJS legacy features, making migration a potential headache. I think the semantics argument (with regard to subscriptions after complete(), publishReplay should behave just like a ReplaySubject and publishBehavior should behave just like a BehaviorSubject) is straightforward and also aligned with traditional Rx, such as in Rx.NET: check Lee Campbells book on this part.

Frikki commented 9 years ago

Sadly, a community fork seems to be the case. :( To label the behavior a corner case is no different than labeling RxJS 5 a corner case. Good luck!

donaldpipowitch commented 9 years ago

:(

jhusain commented 9 years ago

Seems like this issue shouldn't cause a fork. We can leave the existing multicast operator backward-compatible. In my experience the most common indirect use of multi cast is in the following scenarios:

share side effects to avoid concurrent identical network requests
same as the previous scenario except once the data has been retrieved it is forever cached in the Observable

As long as there are two operators that make these scenarios easy, I don't really care what the multicast operator does.

I propose we change share() and avoid using the multicast operator to allow it to be retried. Given it is so commonly used for network requests, this seems like the right decision.

The shareReplay operator doesn't need retry because it's results are cached. It would seem to satisfy the second scenario.

trxcllnt commented 9 years ago

To be clear: the new behavior isn't correct because @blesh says so, it's correct because it's referentially transparent. Connecting a ConnectableObservable should behave the same way every time, regardless of whether you've connected it before or not.

We also have the option to change multicast to a polymorphic function signature: pass it a function, and it'll re-create the inner Subject on every connection; pass it a Subject instance, and it'll always use that instead. @blesh I know you want to minimize polymorphic operators, but it may be advantageous to make an exception here.

benjchristensen commented 9 years ago

We also have the option to change multicastto a polymorphic function signature: pass it a function, and it'll re-create the inner Subject on every connection; pass it a Subject instance, and it'll always use that instead.

This is a good middle-ground and the type of thing we've done in RxJava.

mattpodwysocki commented 9 years ago

@trxcllnt no, that's not the way it was designed nor intended to be designed, unless you happened to be in the original design meetings. It's YOUR opinion as to whether it was right or wrong.

trxcllnt commented 9 years ago

@mattpodwysocki that's exactly my point: it's not "correct" because someone decided so. It's correct because it's functionally pure, and the behavior is consistent with the rest of the library.

The Observable referential transparency is beautiful, allowing them to be retried, etc. In the original design, ConnectableObservables aren't referentially transparent; it behaves differently whether you connect it the first time vs. the second.

My argument is the same here as it is in other discussions re: hot-by-default, primarily: if we start from the pure implementation, we can graduate to the impure as-needed. If we start impure, we can never go back to pure.

I'm arguing for this specifically because I've been bitten by repeat/retry on ConnectableObservable failing.

benlesh commented 9 years ago

Ok, so it's going to be a community fork.

OMG drama. Stop. This issue is open for discussion and compromise.

Okay, so here's a proposed compromise:

change publish, publishBehavior and publishReplay to match RxJS 4 semantics.
multicast can be polymorphic and allow for all behaviors.
Have share maintain the RxJS 5 (Next) semantics.
Remove shareBehavior and shareReplay to prevent user confusion.

trxcllnt commented 9 years ago

@benlesh what if share took the subjectFactory and/or Subject arg just like multicast, effectively meaning "multicast with this, then refCount."

benlesh commented 9 years ago

what if share took the subjectFactory and/or Subject arg just like multicast, effectively meaning "multicast with this, then refCount."

I'd prefer that share() was a simple call. I view it as the operator for "the masses" to use to get what they would expect out of a multicast observable (retry-able, repeatable, etc)

TylorS commented 9 years ago

I'm no expert of the inner workings of Rx, but in my attempts to update a library from Rx 4.0.6 to Rx 5.0.0-alpha.6 I was unable to recreate the behavior that I came to expect regarding replay() and connect(). Again, I know I'm no expert. I know for sure that, as simply a consumer of the great work you are all creating (seriously thank you!), I would have never come up with "solution" proposed above on my own.

benlesh commented 9 years ago

@TylorS ... my proposal above would bring back that behavior.

Out of curiousity, what exact behavior are you relying on, and why are you relying on it?

staltz commented 9 years ago

Okay, so here's a proposed compromise:

change publish, publishBehavior and publishReplay to match RxJS 4 semantics. multicast can be polymorphic and allow for all behaviors. Have share maintain the RxJS 5 (Next) semantics. Remove shareBehavior and shareReplay to prevent user confusion.

Correct, let's do that and sorry about the community fork comment.

benlesh commented 9 years ago