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

WIP: Add static multicasters #5432

Closed
wants to merge 1 commit into from

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented May 14, 2020

This is another go at adding proposed static multicast functions. Returning ConnectableObservable from operators is/was a mistake.

TODO:

  • Settle on names (currently, multicastFrom (static), multicastWith (operator), publishFrom, etc)
  • Deprecate publish operator variants that do not have a supplied selector
  • Add tests
  • Add more docs

NOTE:

Also adds a test showing a weakness of our current approach with regards to composing custom observable types through operator chains. This could probably get added at another time, but I wanted to be sure to capture it while I was thinking of it, and seeing some of the ConnectableObservable code reminded me of the issue.

To discuss:

  • Names? xWith makes sense, but what about xFrom?
  • Should we have a static variant for every permutation, or just for multicast?
  • How frequently do we use these things?

@benlesh
Copy link
Member Author

benlesh commented May 14, 2020

Related #5431

@benlesh benlesh force-pushed the static-publish-variants branch from e90dde0 to 134bc87 Compare May 14, 2020 05:03
@benlesh benlesh force-pushed the static-publish-variants branch from 134bc87 to e4fdd3f Compare May 14, 2020 05:05
Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

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

LGTM. I like the names and the approach. This ought to make the transition easier than it would have been if multicast had been made static without being renamed, etc.

I'll have another look when this is no longer a WIP.

@jgbpercy
Copy link

Related #3833

@benlesh
Copy link
Member Author

benlesh commented Aug 5, 2020

Closing in favor of #5634

@benlesh benlesh closed this Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AGENDA ITEM Flagged for discussion at core team meetings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants