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

Why is there No combineLatestWith counterpart to WithLatestFrom ??? #3831

Closed
Sheshlok opened this issue Apr 4, 2016 · 15 comments
Closed

Why is there No combineLatestWith counterpart to WithLatestFrom ??? #3831

Sheshlok opened this issue Apr 4, 2016 · 15 comments

Comments

@Sheshlok
Copy link

Sheshlok commented Apr 4, 2016

Hi - I am facing an issue where the source observable completes before the other observable starts emitting items. The marble diagram of 'combineLatest' seems best suited and VERY DIFFERENT for that purpose as against 'withLatestFrom'. Using Observable.combineLatest achieves the purpose, although via a much longer route, as it leads to writing multiple 'Observable.combineLatest' functions (as 2 of them cannot be chained together).

Is there a way I can chain several 'combineLatest' operators (in a similar way that I can chain several 'withLatestFrom' operators). Can you help ?
On a side note, do you have any advice on how to deal with the onCompleted state of 1 of the observables when we are combining 2 or more observables, if I face similar problems while chaining operators in future ? Breaking the chain into multiple function calls seems very anti-elegant.

1. Note in this particular use case described above, withLatestFrom behaves very differently from CombineLatest as evident from marble diagrams. Essentially, it does not emit any item at all, if the
other observable (a longer chained call for instance) returns later than source observable.
2. I cannot use 'zipWith' as I need to reflect latest changes in UI whenever any observable emits an item

screen shot 2016-04-05 at 4 42 24 am

screen shot 2016-04-05 at 4 42 36 am

@akarnokd
Copy link
Member

akarnokd commented Apr 5, 2016

CombineLatest and withLatestFrom have different purpose. We don't have an instance version of combineLatest because nobody has needed one up to now. PR welcome.

@Sheshlok
Copy link
Author

Sheshlok commented Apr 5, 2016

Hi Akarnokd - How would you go about writing such an instance version ?
Also, is there any other operator that can achieve the same purpose ?

@akarnokd
Copy link
Member

akarnokd commented Apr 5, 2016

Just look at how zipWith is implemented for inspiration; this operator is an easy one.

@akarnokd
Copy link
Member

See #3966 .

@akarnokd
Copy link
Member

Fix merged in #3966.

@Sheshlok
Copy link
Author

Thanks a ton, David !!!

On Saturday 18 June 2016, David Karnok notifications@github.com wrote:

Fix merged in #3966 #3966.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#3831 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AM7dg7EBJGLG_wblOw2fNBt2hk2B0N5Qks5qMwPggaJpZM4H_mw1
.

Sent from Gmail Mobile

@Sheshlok
Copy link
Author

Hi David - Sorry, I did not check GitHub before and was under the impression that this feature was added from the email notification.
Just had a look at #3966 as you suggested. Have 2 questions:

  1. Is 'withLatestFromMany' intended to function the same as the requested 'combineLatestWith' ?
  2. This feature will be available in 1.7, right ??

@akarnokd
Copy link
Member

  1. No.
  2. Apparently this issue wanted combineLatestWith and not withLatestFromMany.

Feel free to reopen this issue, or better yet, post a PR.

@kevmo314
Copy link

Hi, I have a use case for combineLatestWith, I'd like to emit a protobuf builder both when all the fields are ready and when a field is updated, the API usage would look something like this:

Flowable.fromCallable(Proto::newBuilder)
  .combineLatestWith(foo, Proto.Builder::setFoo)
  .combineLatestWith(bar, Proto.Builder::setBar)
  ...
  .map(Proto.Builder::build);

This ends up being much cleaner than having a massive combineLatest or anything else I've been able to come up with as each op operates on only a single field. I'll send a PR for this shortly. :)

@akarnokd
Copy link
Member

I'm not convinced this type of convenience method is worth it. Also it requires fair amount of adjusting the documentation, adding specific unit tests for both Flowable and Observable for a traditionally static method.

@kevmo314
Copy link

kevmo314 commented Jun 21, 2018

Do you have an alternate proposal for the above builder construction that I could use? Things start to break down and become unmaintainable if there are more than a couple fields.

ie I'm happy to use another approach but atm this is less of a convenience method for my case and more of a maintainability headache reducer.

@akarnokd
Copy link
Member

Have you tried with the static method? Assign them to a local variables with some spacing before and after.

Alternatively, create a transformer factoring out the static call and compose with that.

@kevmo314
Copy link

kevmo314 commented Jun 21, 2018

Is this what you were thinking wrt the static method?

Flowable<Proto.Builder> builder = Flowable.fromCallable(Proto::newBuilder);
Flowable.combineLatest(builder, foo, Proto.Builder::setFoo);
Flowable.combineLatest(builder, bar, Proto.Builder::setBar);
builder.map(Proto.Builder::build());

I suppose this could work, but it seems a bit counterintuitive because they're not chained together. Doesn't this also work somewhat against the whole operator chaining ideal?

@akarnokd
Copy link
Member

You missed the local variable part:

Flowable<Proto.Builder> builder = Flowable.fromCallable(Proto::newBuilder);
Flowable<Proto.Builder> withFoo = Flowable.combineLatest(builder, foo, Proto.Builder::setFoo);
Flowable<Proto.Builder> withBar = Flowable.combineLatest(withFoo, bar, Proto.Builder::setBar);

return withBar.map(Proto.Builder::build());

or the transformer part the article also suggests:

static <T, U, R> FlowableTransformer<T, R> combineLatestWith(Flowable<U> other, 
        BiFunction<? super T, ? super U, ? extends R> combiner) {
    return f -> Flowable.combineLatest(f, other, combiner);
}

Flowable.fromCallable(Proto::newBuilder)
  .compose(combineLatestWith(foo, Proto.Builder::setFoo))
  .compose(combineLatestWith(bar, Proto.Builder::setBar))
  ...
  .map(Proto.Builder::build);

@kevmo314
Copy link

I see. Yeah not convinced by the local variable solution, that seems like a pain to maintain as the wiring order matters.

We can just do the transformer approach then if this shouldn't be in RxJava itself. Thanks!

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

No branches or pull requests

3 participants