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

feat(shareReplay): add config parameter #4059

Merged
merged 9 commits into from Jan 30, 2019
Merged

Conversation

@cartant
Copy link
Collaborator

cartant commented Aug 28, 2018

Description:

This PR adds a config parameter to the shareReplay operator so that it's possible to specify the ref counting behaviour for the use cases outlined in this comment.

If the config parameter is not specified, it defaults to the current behaviour: the subscription to the source is not reference counted and the unsubscriptions from the shared observable do not effect an unsubscription from the source. That is, unsubscription from the source only occurs if the source completes or errors.

I've named the config property refCount, as that seems appropriate, given that the current implementation of the shareReplay operator is not ref counted.

Inline with the comment linked above, if { refCount: true } is specified and the ref count drops to zero, a new ReplaySubject will be created if another subscription to the shared observable is made.

I've added tests for the default behaviour and for { refCount: true } and { refCount: false }.

Also, there was another problem with the previous implementation: the returned teardown function was never called. Instead of being returned, it needs to be added to the Subscriber via the this context.

Related issue (if exists): #3336 #3127


Here's some more detail about the history behind this feature/fix:

  • In 5.4.3 the shareReplay implementation used multicast, but did not recycle the subject if it had completed. That meant that if the reference count dropped to zero, the subject was discarded and the subscription to the source would be unsubscribed.
  • In 5.5.0.beta.4 - in response to this issue - the implementation was changed. And the new implementation essentially no longer reference counted the subscription to the source and that subscription was only unsubscribed upon the source completing or erroring. That is, an explicit unsubscription from the shared observable would never effect an unsubscription from the source.
  • The inability to effect an unsubscription from the source by unsubscribing from the shared observable is something that users have found surprising - see the issues linked above - and it was also a breaking change.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 28, 2018

Pull Request Test Coverage Report for Build 7850

  • 12 of 12 (100.0%) changed or added relevant lines in 1 file are covered.
  • 5 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.2%) to 97.036%

Files with Coverage Reduction New Missed Lines %
src/internal/observable/fromPromise.ts 1 87.5%
src/internal/observable/fromIterable.ts 2 95.24%
src/internal/observable/fromObservable.ts 2 83.33%
Totals Coverage Status
Change from base Build 7849: 0.2%
Covered Lines: 5795
Relevant Lines: 5972

💛 - Coveralls
@cartant

This comment has been minimized.

Copy link
Collaborator Author

cartant commented Aug 28, 2018

Supporting additional overload signatures is possible. More could be added to allow for

shareReplay({ refCount: true })

or

shareReplay(1, { refCount: true })

But I'll wait to see what people think of the PR before adding any. And I'll add some dtslint tests, too, if this is going to be merged.

Copy link
Member

benlesh left a comment

I like the spirit of this one... I think I'd rather force users into a "use the config object, or don't" sort of api:

For example:

source.pipe(
  shareReplay({
    bufferSize: 3,
    windowSize: 2000,
    scheduler: animationFrameScheduler,
    refCount: true,
 }),
)

We can, of course, support the current signature, so it's not a breaking change, but anytime we're over two arguments, I think a configuration object just makes it more readable.

@cartant cartant force-pushed the cartant:issue-3336 branch from e73baa2 to 3b4b01f Aug 29, 2018
@cartant

This comment has been minimized.

Copy link
Collaborator Author

cartant commented Aug 29, 2018

@benlesh I've updated the PR:

  • The config parameter is now used for all parameters and the old signature has been retained as an overload signature.
  • The old signature has been marked as deprecated.
  • When the old signature is used, refCount will be false - inline with the current behaviour.
  • When using the new signature, the config parameter's refCount property is deliberately not optional, so callers will have to specify it. Doing so means that you could remove the old, deprecated signature in v7 and also make the refCount property optional with a default of true - which I think is inarguably the safest and least surprising default behaviour.
  • I've made the same changes to the compat signatures.
  • I've added some dtslint tests.

However, there is one thing about which I'm unsure: I don't know the syntax that should be used to express the config parameter and its properties in the TSDoc. Can anyone help here? @JWO719 ?

@pertrai1

This comment has been minimized.

Copy link
Contributor

pertrai1 commented Aug 29, 2018

@cartant does this answer help with your question about how to document the config object? https://stackoverflow.com/questions/6460604/how-to-describe-object-arguments-in-jsdoc

@cartant

This comment has been minimized.

Copy link
Collaborator Author

cartant commented Aug 29, 2018

@pertrai1 Yeah, that helps. Thanks.

@cartant

This comment has been minimized.

Copy link
Collaborator Author

cartant commented Aug 29, 2018

@benlesh benlesh added the Agenda Item label Aug 29, 2018
@benlesh

This comment has been minimized.

Copy link
Member

benlesh commented Aug 29, 2018

From the core team meeting

Sticking with this PR, we might default to refCount: true in an upcoming major release.

@benlesh benlesh removed the Agenda Item label Aug 29, 2018
@JWO719

This comment has been minimized.

Copy link
Contributor

JWO719 commented Aug 29, 2018

@cartant regarding the documentation. So a basic description of the parameter itself would be totally fine. As far as I know we automatically we link to an api page if it exists, which isn't the case in the current implementation and to be honest I don't know how we want to export the interface.

So basically we just render docs for eported stuff from following files:

  • 'index.ts',
  • 'operators/index.ts',
  • 'ajax/index.ts',
  • 'webSocket/index.ts',
  • 'testing/index.ts'
@cartant

This comment has been minimized.

Copy link
Collaborator Author

cartant commented Aug 29, 2018

@benlesh Regarding this PR being merged/released, I think the docs are pretty important. I agree with OJ that it is or, at least, has the potential to be confusing.

There are some issues - hinted at above - regarding the TSDoc. TypeScript does not like the JSDoc dot-separated style and annotating a separate type is recommended. However, here, ShareReplayConfig is internal. And, also, I don't know how it will work with the generation of the docs app. There's probably as much work involved in that as in the coding.

TL;DR I don't think this can be released without good docs. And I'm going to be working on those, later today.

@benlesh

This comment has been minimized.

Copy link
Member

benlesh commented Aug 29, 2018

@cartant ... looking in Google3, there are a LOT... a LOT LOT of uses of shareReplay where it's just shareReplay(1). I don't think we can deprecate that signature.

Also I think that we should use the TestScheduler marble diagrams for this test.

Copy link
Member

benlesh left a comment

  • Update the tests to use marble diagrams
  • undeprecate the old signature. We'll need to keep it, there are too many uses of shareReplay(1). We can discuss deprecating the "larger" signatures, though.
@benlesh

This comment has been minimized.

Copy link
Member

benlesh commented Aug 29, 2018

Also, once this is done, we'll need to implement the changes in the experimental branch.

@cartant

This comment has been minimized.

Copy link
Collaborator Author

cartant commented Aug 29, 2018

Regarding the deprecation, yeah, I can see that doing so might be too harsh - even though it's not technically a breaking change. One of the reasons for this PR is that I was about to write a TSLint rule to highlight shareReplay's surprising behaviour and thought the effort would be better spent here. However, I can see now that a rule is going to be needed anyway.

@benlesh

This comment has been minimized.

Copy link
Member

benlesh commented Sep 7, 2018

Ping @cartant ... not sure where this one is at right now.

@cartant

This comment has been minimized.

Copy link
Collaborator Author

cartant commented Sep 7, 2018

@benlesh It's on my TODO list. I've not yet addressed your change requests.

@cartant cartant force-pushed the cartant:issue-3336 branch from 318e2bf to e73a5f4 Sep 8, 2018
@cartant

This comment has been minimized.

Copy link
Collaborator Author

cartant commented Sep 8, 2018

@benlesh This is done, except for the docs.

  • I don't know enough about TSDoc and I'm not sure how the approach that TypeScript recommends for config-style parameter types will work - as the config type is internal.
  • Nor am I familiar with the mechanisms - if there are any - for documenting wildly different overload signatures.
  • I'm also not sure how this will affect the docs generation.

With those points in mind, I've made no attempt at updating the TSDoc.

ns-codereview pushed a commit to couchbase/ns_server that referenced this pull request Oct 23, 2018
since ShareReplay doesn't work as expected after upgrade of Rx.js.
it should be fixed soon (ReactiveX/rxjs#4059)

Change-Id: I8c7f4983daf9334095cb9da2f4261060ddf3da89
Reviewed-on: http://review.couchbase.org/100894
Well-Formed: Build Bot <build@couchbase.com>
Reviewed-by: Pavel Blagodov <stochmail@gmail.com>
Tested-by: Pavel Blagodov <stochmail@gmail.com>
@thekiba

This comment has been minimized.

Copy link

thekiba commented Nov 2, 2018

If set { refCount: true } as default, this will be the expected behavior.
@benlesh, what do you think about it?

@@ -1,11 +1,17 @@
import { Observable, SchedulerLike } from 'rxjs';
import { shareReplay as higherOrder } from 'rxjs/operators';
import { ShareReplayConfig } from 'rxjs/internal-compatibility';

/**
* @method shareReplay
* @owner Observable

This comment has been minimized.

Copy link
@benlesh

benlesh Dec 19, 2018

Member

We can remove @owner Observable here. It's not necessary.

This comment has been minimized.

Copy link
@cartant

cartant Dec 19, 2018

Author Collaborator

Done.

@cartant cartant force-pushed the cartant:issue-3336 branch from 0c03047 to a813fc0 Jan 7, 2019
@pauldraper

This comment has been minimized.

Copy link

pauldraper commented Jan 17, 2019

What exactly is the argument for making shareReplay configurably leak subscriptions?

@cartant

This comment has been minimized.

Copy link
Collaborator Author

cartant commented Jan 17, 2019

@pauldraper For background, you could read the issues linked in this PR's description. The discussion therein is close to exhaustive.

What exactly is the argument for making shareReplay configurably leak subscriptions?

See this comment in particular.

@benlesh benlesh merged commit 0fd8707 into ReactiveX:master Jan 30, 2019
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 increased (+0.2%) to 97.036%
Details
@yjaaidi

This comment has been minimized.

Copy link

yjaaidi commented Jan 30, 2019

YEAY!!! 🎉
Good job!

@lock lock bot locked as resolved and limited conversation to collaborators Mar 1, 2019
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.

None yet

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