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 variants returning ConnectableObservable not properly utilizing lift #6003

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Feb 8, 2021

  • Adds tests to show fixed issues
  • Adds tests to show issues that simply can't be fixed, and support a case against removing operators that return ConnectableObservable, as well as possibly a case against lifting Subject.
  • Moves logic that patched lift for ConnectableObservable to the constructor so it is used by all multicast operators

@benlesh benlesh requested a review from cartant February 8, 2021 03:38
…ilizing lift

- Adds tests to show fixed issues
- Adds tests to show issues that simply can't be fixed, and support a case against removing operators that return ConnectableObservable, as well as possibly a case against lifting Subject.
- Moves logic that patched lift for ConnectableObservable to the constructor so it is used by all multicast operators
@benlesh benlesh force-pushed the fix/publish-operators-not-composing-custom-observables branch from 9a011c6 to f722f0a Compare February 8, 2021 05:28
Comment on lines +921 to +924
/**
* Seriously, never do this. It's probably bad that we've allowed this. Fortunately, it's not
* a common practice, so maybe we can remove it?
*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMG, don't give those naughty devs more ideas! 😬 🙈

Comment on lines +960 to +961
* If you're a user and you're reading this... NEVER try to use this feature, it's likely
* to go away at some point.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈

@benlesh benlesh merged commit 9acb950 into ReactiveX:master Feb 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants