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(publish,publishReplay) resolve sharing Subject when subscribing to different source observables #5585

Closed
wants to merge 1 commit into from

Conversation

e-davidson
Copy link
Contributor

Description:

Fixes the issue when using the publish or publishReplay operator in a pipe that's shared with different source observables.

For more details see #5411

  • change publish operator to use factory
  • change publishReplay operator to not share ReplaySubject

Related issue (if exists):

change publish operator to use factory
change publishReplay operator to not share ReplaySubject
fixes issue ReactiveX#5411
@cartant
Copy link
Collaborator

cartant commented Jul 14, 2020

This behaviour is by design and these changes would be breaking.

@e-davidson
Copy link
Contributor Author

I'd be happy to make it backwards compatible. It's unclear where the breaking change would occur since the tests didn’t break. Would you be able to point me in the right direction?

@cartant
Copy link
Collaborator

cartant commented Jul 14, 2020

The change to src/internal/operators/publishReplay.ts looks to me to be breaking. Changing it to use a factory will effect different behaviour, AFAICT.

In any case, I'm these APIs are likely to be deprecated and replaced. See #5432, #5431 and other issues/PRs linked therein.

@cartant
Copy link
Collaborator

cartant commented May 19, 2021

I shouldn't have blocked this, @e-davidson

It does significantly change behaviour, but it's not behaviour that anyone could reasonably be relying upon - so it's more of a bug fix than a breaking change. At least that's what I'm thinking ATM, so we should be able to introduce this fix into v7 (and maybe backport it to v6).

I've cherry-picked your commits into a separate PR. I've resolved the conflicts and I've made similar changes to other operators that have the same issue. Thanks for your work. And your patience.

Closing this to address it in #6410 - which includes your work.

@cartant cartant closed this May 19, 2021
@e-davidson
Copy link
Contributor Author

@cartant
Thank you for moving this forward and I'm glad I was able to assist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants