fix(shareReplay): properly retains history on subscribe

[benlesh]

When the refCount hits zero, and the source has neither completed nor errored, shareReplay will now properly stay subscribed to the source, and retain the internal ReplaySubject that is caching values

fixes #2908

[rxjs-bot]

	Messages
📖

CJS: 1349.3KB, global: 747.9KB (gzipped: 140.6KB), min: 145.5KB (gzipped: 30.8KB)

Generated by 🚫 dangerJS

[coveralls]

Coverage decreased (-0.009%) to 97.456% when pulling 75efb00 on benlesh:fix_shareReplay into 124e231 on ReactiveX:master.

[mgrinko]

@benlesh
As I thought before

.shareReplay(1) === .publishReplay(1).refCount()

But now you did it like

.shareReplay(1) === .publishReplay(1)

Are you sure it is correct???

I think unsubscribing even without error and completion was the only difference between share and publish

[Airblader]

I absolutely think the new behavior is better. If you want the inner subscription to end, just complete the observable. This new behavior allows caching values even if there's no subscriber to the cache for a while.

[mgrinko]

But why can't you use publishReplay when you need to keep inner Subject alive???

It was the only difference between then to close inner subject when there is no more subscribers
https://egghead.io/lessons/rxjs-multicasting-shortcuts-publish-and-variants

And there was a technique to reuse shared observable
https://egghead.io/lessons/rxjs-reusable-multicasting-with-subject-factories

So this change destroyed all described in these videos by @staltz

[Airblader]

But why can't you use publishReplay when you need to keep inner Subject alive???

Because it takes care of neither subscribing the inner observable when a subscription to the outer one is made, nor does it take care of destroying the inner connection on errors, but instead the error will be cached.

[benlesh]

The internal Subject is an implementation detail and shouldn't be considered when using publishReplay. If some other specific behavior is required with regard to Subjects, you can always roll your own operator and use the multicast operator inside of it.

[mgrinko]

@benlesh Thanks we already did so
But I don't understand what is the difference of publishReaplay and shareReplay now
If you could give me the link it would be perfect!

[Airblader]

publishReplay doesn't connect when the ref count goes from 0 to 1. However, publishReplay+refCount will unsubscribe from the source when the ref count goes to 0, while shareReplay doesn't.

Also, publishReplay will cache errors.

[mgrinko]

@Airblader thank you!

[lorenzodallavecchia]

I encountered this behavior today and I'm uncertain about whether it is good or not.

The typical use I have for shareReplay and similar patterns is to provide a shared view of a "costly" value, obtained from a chain of operators that build complex objects that do not need to be re-created for each new observer.

The following is what I usually want.

• The first observer "activates" the upstream part of the operators chain, computing the costly value,
• Subsequent observers do not cause recomputation of the costrly value,
• After the last observer unsubscribes, the upstream operators are "deactivated" (i.e., unsubscribed).
• While there are observers, an emission from the source observable (behind shareReplay and behind all the operators) causes recomputation of the costly objects only once.
• While there are no observers, an emission from the source observable does not recompute nothing (i.e. there are no subscriptions in the operators between the source and shareReplay).
• An observer that arrives after all others have unsubscribed causes another activation of the upstream operators, recomputing the costly value.

With the old behavior, shareReplay(1) (or, equivalently, publishReplay(1).refCount() would do the trick.
With this PR, I now have a problem: the upstream part of the chain keeps recomputing values when there are no observers, instead of recomputing it only once when/if someone subscribes again.

What is the use case for the new behavior?
The only thing I can think of is that, when the source behind shareReplay is "hot", it allows keeping the last value around. For "cold" sources however it just causes useless computations.

For now I can revert to using publishReplay(1).refCount(), so this is not a big deal.
The thing that bothers me is the lack of documentation and the missing of a dual counterpart to shareReplay that does not keep subscriptions (publishReplay is not a dual since it does not imply reference counting).

[Airblader]

What is the use case for the new behavior?

In my case, caching a value even if all observers go away for a while. There's no reason to recompute the costly information just because momentarily all observers go away.

Given that your usecase is solved with a single refCount I just really don't see the need for a change.

The lack of documentation is a general issue, but there is a separate project to tackle this already.

[benlesh]

With this PR, I now have a problem: the upstream part of the chain keeps recomputing values when there are no observers, instead of recomputing it only once when/if someone subscribes again.

@lorenzodallavecchia can you file an issue with a minimal reproduction of what your problem is? Also maybe say in there what you are trying to accomplish?

[mgrinko]

https://codepen.io/anon/pen/qxoNeV?editors=0012
Changing .shareReplay(1) to .publishReplay(1).refCount() of course helps.

[Airblader]

The behavior in that codepen seems perfectly reasonable to me. The source interval is still emitting, so each of its emissions gets passed into the shared ReplaySubject, executing the side effect. The takeUntil happens after shareReplay, so I don't see why that should affect what happens in the multicast observable.

Changing .shareReplay(1) to .publishReplay(1).refCount() of course helps.

So where's the problem if you have a working solution?

[lorenzodallavecchia]

@lorenzodallavecchia can you file an issue with a minimal reproduction of what your problem is?

The codepen posted by @mgrinko represents my use case too.

To clarify, I agree that this works correctly according to the new design. With my comment here I wanted to point out that the change in the behavior of shareReplay is something that could cause subtle performance problems in code that was counting on the operator to unsubscribe. I would have expected a change like this to be rolled out in a major version as a breaking change.

And that's basically it. The rest is just related issues/ideas, like having a "cold" equivalent to shareReplay and of course, as it was said already, better docs.

[mgrinko]

The problem is that in 5.4 version of RxJS worked differently
Here is the same code but with RxJS 5.4.3

https://codepen.io/anon/pen/zRWoMV?editors=0012

So now we have tons of code which relies on old behavior,
but now we need to fix it all after update

[Airblader]

So now we have tons of code which relies on old behavior

@benlesh titled this a bugfix, so apparently this old behavior was never intended. It's the nature of relying on buggy behavior that a bugfix might break your usecase. The real problem, I guess, is that the docs weren't clear enough to state how it should've behaved.

https://xkcd.com/1172/

[mgrinko]

agree

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