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

Transforming and Combining operators for PrimitiveSequence #1248

Closed
2 of 9 tasks
hagmas opened this issue May 12, 2017 · 3 comments
Closed
2 of 9 tasks

Transforming and Combining operators for PrimitiveSequence #1248

hagmas opened this issue May 12, 2017 · 3 comments

Comments

@hagmas
Copy link
Contributor

hagmas commented May 12, 2017

Short description of the issue:

Some transforming and combining operators needs to be added to PrimitiveSequence, and some should be removed because they have no effects on some traits, such as FlatMap and Map for Completable.

Related Issues:

#1206
#1245

Expected outcome:

Here is a table that describes the relationship between some operators and PrimitiveSequence traits. ⭕️ means the operator has effect on the trait and ❌ means ineffective.

Operators Single Maybe Completable
AsMaybe ⭕️ ⭕️
AsSingle
AsCompletable ⭕️
Combine ⭕️(Same as Zip) ⭕️(Same as Zip) ⭕️(Same as Zip)
Concat ⭕️(*1) ⭕️
Map ⭕️ ⭕️
Merge ⭕️ ⭕️
Switch, FlatMap ⭕️ ⭕️
SwitchIfEmpty ⭕️ ⭕️(Same as Concat)
Zip ⭕️ ⭕️ ⭕️(*2)

*1: Concat for Maybe is effective only when either of the observables is completed.
*2: Zip for Completable is effective but a useless resultSelector needs to be defined. This can be replaced with Merge operator.

To implement these new transforming and combining operators for PrimitiveSequence, I would suggest to define them separately in three files "Single+Operators", "Maybe+Operators" and "Completable+Operators", e.g., defining FlatMap operator in "Single+Operators" and "Maybe+Operators", but not in "Completable+Operators". Operators valid for all the traits such as Combine or other creating/utility operators can be defined in the "PrimitiveSequence.swift" file as currently defined.

What actually happens:
The actual issue with the current version of code is that, Map and FlatMap can be used for Completable because they are defined in the extension of PrimitiveSequence. However, actually they have no effect and can be misleading.

Completable.empty().flatMap { _ in
    // the flow never comes here.
    return Completable.empty()
}.subscribe().disposed(by: disposeBag)

RxJava defines all the operators separately for all the traits and avoids this kind of situation.

Also, some combining and transforming operators should be added for usability because to use some operators we need to convert PrimitiveSequence to observable once which increase redundancy in the code.

let secondCompletable = Completable.empty().do(onCompleted: {
    // do something
})
Completable.empty().asObservable().concat(secondCompletable.asObservable()).subscribe().disposed(by: disposeBag)

RxSwift/RxCocoa/RxBlocking/RxTest version/commit

3.4.1

Platform/Environment

  • iOS
  • macOS
  • tvOS
  • watchOS
  • playgrounds

How easy is to reproduce? (chances of successful reproduce after running the self contained code)

  • easy, 100% repro
  • sometimes, 10%-100%
  • hard, 2% - 10%
  • extremely hard, %0 - 2%

Xcode version:

8.3.2
@kzaher
Copy link
Member

kzaher commented May 12, 2017

Hi @hagmas ,

I agree with most of what you've said. We need to continue improving primitive sequence derived traits.

I'm not that concerned at the moment that Completable has map because we can ofc deprecate it on Completable in next major version of RxSwift. Yes, I agree, it's not perfect.

I understand that RxJava defines all of the operators for each unit completely separate and optimized, but since we didn't notice any performance issues my biggest concern is to make the project as small and maintainable as possible.

I also seemed to me that it would be cool to just present how all of these concepts can be built on top of observable sequence APIs :)

I think we need to tackle one item at a time. I would rather first fill out the missing functionalities and leave removal and polishing of redundant ones for next major update. They aren't perfect, but they also don't do much harm.

If you are willing to help with those items, feel free to suggest what to tackle first. If we start working on all of those things in that list at once, we will make one giant mess that will be hard to CR.

Maybe temporarily adding merge to Completable to fix #1245. I would like to rename, merge on Completable to zip on major version upgrade and really fix the issue with zip result selector.

Then adding flatMapAsCompletable, flatMapAsSingle, flatMapAsMaybe ...

Then adding asMaybe, asSingle, asCompletable ....

etc ...

@hagmas
Copy link
Contributor Author

hagmas commented May 13, 2017

Hello @kzaher ,

Thank you for replying. What you said sounds like a plan.

After I looked back all the operators, probably Merge and Concat for Completable are one of the most useful ones so I am going start tackling on them. And I will leave the rest for your plan.

@kzaher
Copy link
Member

kzaher commented Sep 17, 2017

This should be solved now.

@kzaher kzaher closed this as completed Sep 17, 2017
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

No branches or pull requests

2 participants