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

add ValueObservable.combineLatestN + tests #337

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lukepighetti
Copy link

Quick PR just to see what your appetite is for ValueObservable specific methods to preserve ValueObservable type

@brianegan
Copy link
Collaborator

I actually really like the idea behind this change. It's something I'd considered in the past as well!

One question about the implementation that I'd love some feedback on: It currently uses shareValueSeeded to achieve the effect. That will mean the Observable will become unusable after the final listener unsubscribes to it. Would that be the expected behavior for peeps using this code?

@lukepighetti
Copy link
Author

I was not aware that was a side-effect of using shareValueSeeded. The purpose for using it was to try to preserve value at all costs.

@brianegan
Copy link
Collaborator

Heya @lukepighetti -- how are ya feeling about this change?

@lukepighetti
Copy link
Author

lukepighetti commented Nov 25, 2019

If you can provide a quick and clear request for the change I'd be happy to implement it and push. My main concern is making sure this is consistent with your vision for rxdart

@brianegan
Copy link
Collaborator

brianegan commented Nov 26, 2019

One thing I was considering: I don't think most people want shareValueSeeded (from the Rx spec), which will shut down when there are no more listeners. Rather, I think most folks want an operator that works like asBroadcastStream but also captures the latest value. I was thinking of adding two new operators:

asBroadcastValueStream & asBroadcastValueStreamSeeded. These would work exactly like asBroadcastStream -- in fact, they'd use that method under the hood, but would also provide access to the latest emitted value.

Perhaps we could use that base for this change?

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.

2 participants