Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(shareReplay): properly retains history on subscribe #2910

Merged
merged 1 commit into from Oct 6, 2017

Conversation

@benlesh
Copy link
Member

benlesh commented Oct 6, 2017

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

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

This comment has been minimized.

Copy link

rxjs-bot commented Oct 6, 2017

Messages
📖

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

Generated by 🚫 dangerJS

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 6, 2017

Coverage Status

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

@kwonoj
kwonoj approved these changes Oct 6, 2017
@benlesh benlesh merged commit accbcd0 into ReactiveX:master Oct 6, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.009%) to 97.456%
Details
@mgrinko

This comment has been minimized.

Copy link

mgrinko commented Jan 12, 2018

@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

This comment has been minimized.

Copy link
Contributor

Airblader commented Jan 21, 2018

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

This comment has been minimized.

Copy link

mgrinko commented Jan 22, 2018

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

This comment has been minimized.

Copy link
Contributor

Airblader commented Jan 22, 2018

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

This comment has been minimized.

Copy link
Member Author

benlesh commented Jan 23, 2018

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

This comment has been minimized.

Copy link

mgrinko commented Jan 26, 2018

@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

This comment has been minimized.

Copy link
Contributor

Airblader commented Jan 26, 2018

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

This comment has been minimized.

Copy link

mgrinko commented Jan 26, 2018

@Airblader thank you!

@lorenzodallavecchia

This comment has been minimized.

Copy link

lorenzodallavecchia commented Feb 13, 2018

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

This comment has been minimized.

Copy link
Contributor

Airblader commented Feb 13, 2018

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

This comment has been minimized.

Copy link
Member Author

benlesh commented Feb 20, 2018

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

This comment has been minimized.

Copy link

mgrinko commented Feb 20, 2018

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

@Airblader

This comment has been minimized.

Copy link
Contributor

Airblader commented Feb 20, 2018

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

This comment has been minimized.

Copy link

lorenzodallavecchia commented Feb 20, 2018

@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

This comment has been minimized.

Copy link

mgrinko commented Feb 20, 2018

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

This comment has been minimized.

Copy link
Contributor

Airblader commented Feb 20, 2018

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

This comment has been minimized.

Copy link

mgrinko commented Feb 20, 2018

agree

@lock

This comment has been minimized.

Copy link

lock bot commented Jun 6, 2018

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.