🎉 `withLatestFrom` for more than one publisher. #22

jdisho · 2020-04-11T12:10:31Z

I'm working on creating withLatestFrom operators that support more than one publisher.
In RxSwift, I face many scenarios where I need to get the latest values from more than one observable and additionally there is this great thread.

Since under the hood, I'm using combineLatest, I'm supporting as many publishers as the operator supports (up to four).

freak4pc · 2020-04-11T12:41:56Z

Hey @jdisho, first of all thank you so much for your work.

TBH I have some issues with adding something like this:

I agree with the thread you linked that this should be a per-project/repo thing because it is a specific constraint of some, but not all, use cases.
The bigger issue is that this tucks in (e.g hides) the fact combineLatest is used under the hood which usually has no weird side-effects but it still quite strange.

I’m all for keeping this issue open to see if there’s more practical interest in adding this, and also to think a bit more if this is a good fit for this library.

Makes sense to you? WDYT?

jdisho · 2020-04-11T13:26:12Z

Hey @freak4pc, thanks for your explanation. We can leave it open and see if other folks have different opinions. 😄

However, can you elaborate a bit more on your points? B/c, I don't fully understand them. 😇

... because it is a specific constraint of some, but not all, use cases.

What can be some other cases where the implementation with combineLatest can be incorrect or have unexpected side effects?

In an example, don't you always expect on button tap, to get the latest value from publisher1 and publisher2?
```
button.tapPublisher.withLatestFrom(publisher1, publisher2)
```
... the fact combineLatest is used under the hood which usually has no weird side-effects but it still quite strange.

Why do you think this is strange? Is this just your personal opinion? Based on this argument, every new operator that reuses given ones under the hood can feel strange. 🙇‍♂️

jdisho · 2020-04-11T13:44:07Z

What does @jasdev think? 🤓

freak4pc · 2020-04-11T13:45:00Z

Yes, some people don't have the use case of combining multiple inputs this way on a button tap. Also the point the original thread makes is that this is very simplistic sugar that is perhaps left explicit given it doesn't provide any "logic" but just basically removes "CombineLatest" from the call-site. These sugar-y operators (IMO) do require thinking over more carefully if they're actually needed.
Of course it's my personal opinion, I can't have anyone else's opinion but my own :)

jdisho · 2020-04-11T14:02:00Z

IMO, the user of this operator shouldn't care about the implantation details. I think what's important is the operator's description to match the expectations.

If this is an option, we can create WithLatestFrom2 and WithLatestFrom3 without using combineLatest.

freak4pc · 2020-04-11T14:55:02Z

Ok, I thought of it some more, I think this could be an OK addition. I'd be happy to get more opinions though.

freak4pc · 2020-04-11T15:10:26Z

Sources/Operators/WithLatestFrom.swift

+  /// Upon an emission from self, emit the latest value from the
+  /// second and third publisher, if any exists.
+  ///
+  /// - parameter other: A second publisher source.
+  /// - parameter other1: A third publisher source.
+  ///
+  /// - returns: A publisher containing the latest value from the second and third publisher, if any.
+  func withLatestFrom<Other, Other1>(_ other: Other,
+                                     _ other1: Other1)
+    -> AnyPublisher<(Self.Output, Other.Output, Other1.Output), Failure>
+    where Other: Publisher, Other1: Publisher, Other.Failure == Failure, Other1.Failure == Failure {
+      let combined = combineLatest(other, other1)
+        .eraseToAnyPublisher()
+      return withLatestFrom(combined)
+        .eraseToAnyPublisher()
+    }


I think we can go down this road:

Suggested change

/// Upon an emission from self, emit the latest value from the

/// second and third publisher, if any exists.

///

/// - parameter other: A second publisher source.

/// - parameter other1: A third publisher source.

///

/// - returns: A publisher containing the latest value from the second and third publisher, if any.

func withLatestFrom<Other, Other1>(_ other: Other,

_ other1: Other1)

-> AnyPublisher<(Self.Output, Other.Output, Other1.Output), Failure>

where Other: Publisher, Other1: Publisher, Other.Failure == Failure, Other1.Failure == Failure {

let combined = combineLatest(other, other1)

.eraseToAnyPublisher()

return withLatestFrom(combined)

.eraseToAnyPublisher()

}

/// Upon an emission from self, emit the latest value from the

/// second and third publisher, if any exists.

///

/// - parameter other: A second publisher source.

/// - parameter other1: A third publisher source.

///

/// - returns: A publisher containing the latest value from the second and third publisher, if any.

func withLatestFrom<Other: Publisher, Other1: Publisher>(_ other: Other,

_ other1: Other1)

-> Publishers.WithLatestFrom<Self,

AnyPublisher<(Other.Output, Other1.Output), Self.Failure>,

(Self.Output, Other.Output, Other1.Output)>

where Other.Failure == Failure, Other1.Failure == Failure {

let combined = other.combineLatest(other1)

.eraseToAnyPublisher()

return withLatestFrom(combined, resultSelector: { ($0, $1.0, $1.1) })

}

codecov · 2020-04-12T15:58:42Z

Codecov Report

Merging #22 into master will increase coverage by 0.65%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #22      +/-   ##
==========================================
+ Coverage   96.72%   97.37%   +0.65%     
==========================================
  Files          34       38       +4     
  Lines        1617     2246     +629     
==========================================
+ Hits         1564     2187     +623     
- Misses         53       59       +6

Impacted Files	Coverage Δ
Sources/Operators/WithLatestFrom.swift	`93.15% <100.00%> (+1.34%)`	⬆️
Tests/WithLatestFromTests.swift	`100.00% <100.00%> (ø)`
Tests/CreateTests.swift	`100.00% <0.00%> (ø)`
Sources/Subjects/ReplaySubject.swift	`82.35% <0.00%> (ø)`
Tests/ShareReplayTests.swift	`100.00% <0.00%> (ø)`
Sources/Operators/ShareReplay.swift	`100.00% <0.00%> (ø)`
Tests/ReplaySubjectTests.swift	`100.00% <0.00%> (ø)`
Sources/Operators/Create.swift	`100.00% <0.00%> (+12.50%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ed4677...25ba2dc. Read the comment docs.

freak4pc

This looks great! I literally only have a super-tiny comment, you did a really stellar job on this PR 👏👏👏

Very detail-oriented.

Sources/Operators/WithLatestFrom.swift

freak4pc

Oh wait, I just realized you're missing README.md updates :)
Mind taking care of those @jdisho ?

jdisho · 2020-04-16T09:19:52Z

Always forget that! 😄
Yes, I will take care. Thanks for the review.

freak4pc · 2020-04-19T08:14:08Z

Do you have time to finish the README here? I'm happy to wrap it for you. Let me know ;)

jdisho · 2020-04-19T09:45:04Z

Working on it now 🤗

freak4pc · 2020-04-19T09:46:44Z

freak4pc · 2020-04-19T19:27:01Z

README.md

@@ -74,9 +74,11 @@ github "CombineCommunity/CombineExt"

 This section outlines some of the custom operators CombineExt provides.

-### withLatestFrom
+## withLatestFrom


freak4pc · 2020-04-19T19:27:38Z

README.md

-Merges two publishers into a single publisher by combining each value from `self` with the _latest_ value from the second publisher, if any.
+### withLatestFrom(_:)
+
+Merges **two** publishers into a single publisher by combining each value from `self` with the _latest_ value from the second publisher, if any.


Let's not add specific documentation per-overload:)
We should just expand withLatestFrom mentioning it supports overloads of up to 4 observables

I updated te README based on this comment and I hope I got it right. Should it be fine now? 😄

freak4pc

Minor note but looks greattt

README.md

jdisho added 3 commits April 11, 2020 01:26

Support withLatestFrom with multiple publishers

24dded9

Test withLatestFrom with 2 publishers

197584c

Add comment for the new operators

150d6b2

freak4pc requested changes Apr 11, 2020

View reviewed changes

Support different variations of withLatestFrom with a ResultSelector

b5d6cf6

jdisho added 2 commits April 12, 2020 18:06

Test withLatestFrom3 with result selector

861e0cf

Test withLatestFrom2 and 3 with no result selector

8a82d37

jdisho changed the title ~~[WIP] withLatestFrom for more than one publisher.~~ 🎉 withLatestFrom for more than one publisher. Apr 12, 2020

jdisho requested a review from freak4pc April 12, 2020 16:26

freak4pc approved these changes Apr 16, 2020

View reviewed changes

Sources/Operators/WithLatestFrom.swift Outdated Show resolved Hide resolved

Sources/Operators/WithLatestFrom.swift Outdated Show resolved Hide resolved

freak4pc added 2 commits April 16, 2020 12:15

Update Sources/Operators/WithLatestFrom.swift

2a4bd2b

Update Sources/Operators/WithLatestFrom.swift

04bcc5b

freak4pc requested changes Apr 16, 2020

View reviewed changes

jdisho force-pushed the withLatestFrom-many-publishers branch from f52695a to 1826a29 Compare April 19, 2020 11:23

freak4pc requested changes Apr 19, 2020

View reviewed changes

Update withLatestFrom section in the README

eb08832

jdisho force-pushed the withLatestFrom-many-publishers branch from 1826a29 to eb08832 Compare April 20, 2020 08:42

freak4pc requested changes Apr 20, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

Update README.md

25ba2dc

freak4pc merged commit be7e1d0 into CombineCommunity:master Apr 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎉 `withLatestFrom` for more than one publisher. #22

🎉 `withLatestFrom` for more than one publisher. #22

jdisho commented Apr 11, 2020 •

edited

Loading

freak4pc commented Apr 11, 2020

jdisho commented Apr 11, 2020

jdisho commented Apr 11, 2020 •

edited

Loading

freak4pc commented Apr 11, 2020

jdisho commented Apr 11, 2020

freak4pc commented Apr 11, 2020

freak4pc Apr 11, 2020

codecov bot commented Apr 12, 2020 •

edited

Loading

freak4pc left a comment •

edited

Loading

freak4pc left a comment

jdisho commented Apr 16, 2020

freak4pc commented Apr 19, 2020

jdisho commented Apr 19, 2020

freak4pc commented Apr 19, 2020

freak4pc Apr 19, 2020

freak4pc Apr 19, 2020

jdisho Apr 20, 2020

freak4pc left a comment

🎉 withLatestFrom for more than one publisher. #22

🎉 withLatestFrom for more than one publisher. #22

Conversation

jdisho commented Apr 11, 2020 • edited Loading

freak4pc commented Apr 11, 2020

jdisho commented Apr 11, 2020

jdisho commented Apr 11, 2020 • edited Loading

freak4pc commented Apr 11, 2020

jdisho commented Apr 11, 2020

freak4pc commented Apr 11, 2020

freak4pc Apr 11, 2020

Choose a reason for hiding this comment

codecov bot commented Apr 12, 2020 • edited Loading

Codecov Report

freak4pc left a comment • edited Loading

Choose a reason for hiding this comment

freak4pc left a comment

Choose a reason for hiding this comment

jdisho commented Apr 16, 2020

freak4pc commented Apr 19, 2020

jdisho commented Apr 19, 2020

freak4pc commented Apr 19, 2020

freak4pc Apr 19, 2020

Choose a reason for hiding this comment

freak4pc Apr 19, 2020

Choose a reason for hiding this comment

jdisho Apr 20, 2020

Choose a reason for hiding this comment

freak4pc left a comment

Choose a reason for hiding this comment

🎉 `withLatestFrom` for more than one publisher. #22

🎉 `withLatestFrom` for more than one publisher. #22

jdisho commented Apr 11, 2020 •

edited

Loading

jdisho commented Apr 11, 2020 •

edited

Loading

codecov bot commented Apr 12, 2020 •

edited

Loading

freak4pc left a comment •

edited

Loading