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

shareReplay fix does not support lift #2921

Closed
cartant opened this issue Oct 7, 2017 · 8 comments
Closed

shareReplay fix does not support lift #2921

cartant opened this issue Oct 7, 2017 · 8 comments
Assignees

Comments

@cartant
Copy link
Collaborator

cartant commented Oct 7, 2017

The fix made to shareReplay in #2910 sees a non-lifted observable returned. The previous implementation returned an observable lifted from the source.

RxJS version: 5.5.0-beta.5

@kwonoj
Copy link
Member

kwonoj commented Oct 7, 2017

what's exact issue? is there regression occurred?

@cartant
Copy link
Collaborator Author

cartant commented Oct 7, 2017

Yes, the previous implementation used multicast which calls lift on the source observable, allowing a source-determined instance to be returned. However, the fix uses Observable.create and the source's lift is no longer called and a source-determined instance won't be returned.

I can give you example code if you really want it, but I thought the issue was clear. That is, the source's implementation of lift will no longer be called.

@kwonoj
Copy link
Member

kwonoj commented Oct 7, 2017

I recall there were few non lifted around multicasting behavior, but can't recall exactly. /cc @benlesh

@kwonoj
Copy link
Member

kwonoj commented Oct 7, 2017

@cartant

implementation of lift will no longer be called.

yes, that's part I need to confirm that I recall there was few non-lifted around multicast already but I can't recall exactly (I mostly didn't write those)

@cartant
Copy link
Collaborator Author

cartant commented Oct 7, 2017

I just noticed this when looking more closely at the fixed shareReplay in an attempt to determine what its expected behaviour ought to be - as I'd thought the previous, unfixed implementation was the expected behaviour.

Whether or not it should be lifted, I doubt is especially important. And it's not a regression that affects me. But it is a change.

@cartant
Copy link
Collaborator Author

cartant commented Oct 7, 2017

Regarding multicast calling lift: it does. I checked the behaviours of the pre and post fix shareReplay implementations before creating this issue.

@benlesh
Copy link
Member

benlesh commented Oct 9, 2017

FWIW: shareReplay has waffled between supporting lift and not, I think I can fix this before we close the beta.

@benlesh benlesh closed this as completed in 3d9cf87 Oct 9, 2017
@lock
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.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants