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

feat(shareReplay): adds `shareReplay` variant of `publishReplay` #2443

Merged
merged 1 commit into from May 9, 2017

Conversation

Projects
None yet
7 participants
@benlesh
Member

benlesh commented Mar 4, 2017

shareReplay returns an observable that is the source multicasted over a ReplaySubject. That replay subject is recycled on error from the source, but not on completion of the source. This makes shareReplay ideal for handling things like caching AJAX results, as it's retryable. It's repeat behavior, however, differs from share in that it will not repeat the source observable, rather it will repeat the source observable's values.

related #2013, #453, #2043

@benlesh benlesh referenced this pull request Mar 4, 2017

Closed

feat(shareResults): add shareResults operator #2043

3 of 5 tasks complete
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 4, 2017

Coverage Status

Coverage decreased (-0.1%) to 97.592% when pulling f1e8cc0 on benlesh:shareReplay into 7c41c08 on ReactiveX:master.

coveralls commented Mar 4, 2017

Coverage Status

Coverage decreased (-0.1%) to 97.592% when pulling f1e8cc0 on benlesh:shareReplay into 7c41c08 on ReactiveX:master.

feat(shareReplay): adds `shareReplay` variant of `publishReplay`
`shareReplay` returns an observable that is the source multicasted over a `ReplaySubject`. That replay subject is recycled on error from the `source`, but not on completion of the source. This makes `shareReplay` ideal for handling things like caching AJAX results, as it's retryable. It's repeat behavior, however, differs from `share` in that it will not repeat the `source` observable, rather it will repeat the `source` observable's values.

related #2013, #453, #2043
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 4, 2017

Coverage Status

Coverage decreased (-0.009%) to 97.679% when pulling b0f7266 on benlesh:shareReplay into 7c41c08 on ReactiveX:master.

coveralls commented Mar 4, 2017

Coverage Status

Coverage decreased (-0.009%) to 97.679% when pulling b0f7266 on benlesh:shareReplay into 7c41c08 on ReactiveX:master.

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Mar 4, 2017

Member

I suspect the 0.009% change in coverage isn't worth blocking this over, as it's pretty much a rounding error.

Member

benlesh commented Mar 4, 2017

I suspect the 0.009% change in coverage isn't worth blocking this over, as it's pretty much a rounding error.

@robwormald

This comment has been minimized.

Show comment
Hide comment
@robwormald

robwormald Mar 4, 2017

Contributor

It's repeat behavior, however, differs from share in that it will not repeat the source observable, rather it will repeat the source observable's values.

can you expand on the behavior differences? Perhaps with an example?

Contributor

robwormald commented Mar 4, 2017

It's repeat behavior, however, differs from share in that it will not repeat the source observable, rather it will repeat the source observable's values.

can you expand on the behavior differences? Perhaps with an example?

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 4, 2017

Coverage Status

Coverage decreased (-0.1%) to 97.592% when pulling f1e8cc0 on benlesh:shareReplay into 7c41c08 on ReactiveX:master.

coveralls commented Mar 4, 2017

Coverage Status

Coverage decreased (-0.1%) to 97.592% when pulling f1e8cc0 on benlesh:shareReplay into 7c41c08 on ReactiveX:master.

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Mar 4, 2017

Member

can you expand on the behavior differences? Perhaps with an example?

@robwormald The tests show the behavior, but in case they aren't clear enough...

repeat will work exactly the same way repeat works with publishReplay().refCount().

assume that source$ below is using shareReplay()...

--a--b--c--|             // source$
--a--b--c--(abcabc|)     // source$.repeat(3);

retry on the otherhand, will work exactly the same way .multicast(() => new ReplaySubject()).refCount() does.

--a--b--c--#                                      // source$
--a--b--c----a--b--c----a--b--c----a--b--c--#     // source$.retry(3)

... hopefully, that illustrates the behavior?

Member

benlesh commented Mar 4, 2017

can you expand on the behavior differences? Perhaps with an example?

@robwormald The tests show the behavior, but in case they aren't clear enough...

repeat will work exactly the same way repeat works with publishReplay().refCount().

assume that source$ below is using shareReplay()...

--a--b--c--|             // source$
--a--b--c--(abcabc|)     // source$.repeat(3);

retry on the otherhand, will work exactly the same way .multicast(() => new ReplaySubject()).refCount() does.

--a--b--c--#                                      // source$
--a--b--c----a--b--c----a--b--c----a--b--c--#     // source$.retry(3)

... hopefully, that illustrates the behavior?

@trxcllnt

This comment has been minimized.

Show comment
Hide comment
@trxcllnt

trxcllnt Mar 5, 2017

Member

The only reservation I have is that the ReplaySubject isn't deallocated when the refCount Observable is disposed. By referencing the RS in the Observable definition, it'll only be GC'd when the shareReplay Observable is dereferenced. None of the other operators work this way, and it seems this could introduce leaks when people unfamiliar with the implementation use Observables like normal, expecting they teardown when all the Subscriptions are disposed.

Member

trxcllnt commented Mar 5, 2017

The only reservation I have is that the ReplaySubject isn't deallocated when the refCount Observable is disposed. By referencing the RS in the Observable definition, it'll only be GC'd when the shareReplay Observable is dereferenced. None of the other operators work this way, and it seems this could introduce leaks when people unfamiliar with the implementation use Observables like normal, expecting they teardown when all the Subscriptions are disposed.

@wardbell

This comment has been minimized.

Show comment
Hide comment
@wardbell

wardbell Mar 5, 2017

Contributor

This seems promising. How do I use if for the following scenario.

I want to cache a list of customers from the server but periodically refresh that list, say, if it is more than an hour old.

  1. First subscriber triggers the first fetch (in the source$, yes?).
  2. If a second subscriber asks for customers before the first fetch succeeds, it waits until that first fetch succeeds or errors.
  3. What does the second subscriber see if it errors and we retry? Should it wait? Is there a race?
  4. How do I trigger the re-fetch after the cached value expires? That only works if the fetching source (unlike Angular http) does not complete, right?

In the absence of the shareReplay operator, I handle this scenario by managing both the fetching Observable and the ReplaySubject on my own in an imperative manner. Should I continue to do so?

Be gentle ... I'm a comparative noob.

Contributor

wardbell commented Mar 5, 2017

This seems promising. How do I use if for the following scenario.

I want to cache a list of customers from the server but periodically refresh that list, say, if it is more than an hour old.

  1. First subscriber triggers the first fetch (in the source$, yes?).
  2. If a second subscriber asks for customers before the first fetch succeeds, it waits until that first fetch succeeds or errors.
  3. What does the second subscriber see if it errors and we retry? Should it wait? Is there a race?
  4. How do I trigger the re-fetch after the cached value expires? That only works if the fetching source (unlike Angular http) does not complete, right?

In the absence of the shareReplay operator, I handle this scenario by managing both the fetching Observable and the ReplaySubject on my own in an imperative manner. Should I continue to do so?

Be gentle ... I'm a comparative noob.

const subscriber2 = hot(' b| ').mergeMapTo(shared);
const expected2 = ' (12)-3-# ';
const subscriber3 = hot(' (c|) ').mergeMapTo(shared);
const expected3 = ' -1-2-----3-#';

This comment has been minimized.

@staltz

staltz Mar 5, 2017

Member

Shouldn't expected3 be (23)#?

@staltz

staltz Mar 5, 2017

Member

Shouldn't expected3 be (23)#?

This comment has been minimized.

@staltz

staltz Mar 5, 2017

Member

Ignore me. It's correct. (because the source errored, it didn't complete, so we create a new ReplaySubject for the next multicast)

@staltz

staltz Mar 5, 2017

Member

Ignore me. It's correct. (because the source errored, it didn't complete, so we create a new ReplaySubject for the next multicast)

@staltz

This comment has been minimized.

Show comment
Hide comment
@staltz

staltz Mar 5, 2017

Member

Apart from what has already been said (e.g. by Paul), this looks good to merge IMO.

Member

staltz commented Mar 5, 2017

Apart from what has already been said (e.g. by Paul), this looks good to merge IMO.

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Mar 5, 2017

Member

@staltz, in discussing this with @trxcllnt on Slack, I pointed out that the memory behavior here is no different than how any of the publish operators, or multicast(subject) have worked since RxJS 2.

Thought I'd add it to the thread though, for visibility.

Member

benlesh commented Mar 5, 2017

@staltz, in discussing this with @trxcllnt on Slack, I pointed out that the memory behavior here is no different than how any of the publish operators, or multicast(subject) have worked since RxJS 2.

Thought I'd add it to the thread though, for visibility.

@wardbell wardbell referenced this pull request Mar 5, 2017

Merged

feat(aio): add API list page #14899

2 of 7 tasks complete

@benlesh benlesh merged commit 5a2266a into ReactiveX:master May 9, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.009%) to 97.679%
Details
@QuentinFchx

This comment has been minimized.

Show comment
Hide comment
@QuentinFchx

QuentinFchx May 11, 2017

Shall the MIGRATION.md be updated aswell? It might be confusing otherwise

QuentinFchx commented May 11, 2017

Shall the MIGRATION.md be updated aswell? It might be confusing otherwise

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot 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 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.