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

*Breaking changes* Change toArray() to return Single #1543

Closed
lucas34 opened this issue Jan 8, 2018 · 8 comments
Closed

*Breaking changes* Change toArray() to return Single #1543

lucas34 opened this issue Jan 8, 2018 · 8 comments

Comments

@lucas34
Copy link
Contributor

lucas34 commented Jan 8, 2018

Hi,

Current implementation of toArray() method in Observable will return another Observable. Since RxSwift now have all Traits. For me it will make more sense to return Single Instead.

let array: Single<[Int]> = Observable<Int>.just(1, 2, 3).toArray()
Observable<Int>.empty().toArray() // equivalent to Single<Int>.just([])

Unless there is any case that toArray will emit multiple items. But like the documentation says
Converts an Observable into another Observable that emits the whole sequence as a Single array and then terminates.

I'm migrating to Traits to make my streams more representative and more strict. If you agree to have more methods to return another type like some methods in Observable returns something else than Observable (Single, Maybe, Completable) I can suggest more operators.
But if operator in Observable should only return another Observable it's also understandable.

@kzaher
Copy link
Member

kzaher commented Jan 8, 2018

Hi @lucas34 ,

I think your complaint makes sense. It would be great if we could make this a non breaking change somehow.

ToArraySingle looks a bit ugly to me, but maybe there is some obvious and elegant way we can encorporate these changes

@freak4pc
Copy link
Member

For non-breaking I don't think there's much you can do besides either:

  1. You can have asArray that would do single and "phase out" toArray
  2. You can have toSingleArray, but agree this is a tad ugly

@lucas34
Copy link
Contributor Author

lucas34 commented Jan 15, 2018

Actually changing the return type has already been done for ignoreElements.
#1436

I think it would be acceptable to change the return type for the next major release. Like 4.2.0. or 5.X. I'm scare that if we start to have toArray then toSingleArray we will have later toMaybeArray and so on for other operators as well.

@freak4pc
Copy link
Member

freak4pc commented Jan 15, 2018

The only problem I see here is that the most common use case is working with Observable, and not with Traits. (e.g. it's the "common grounds" of all RxSwift users)

Using this sort of "Catchall" that would always return a Single might not be beneficial to people who do not see (or are interested) in the benefits of traits.

Do we prefer people using .asSingle() sometimes or people using .asObservable() sometimes? I guess that's the question.

Anyways If there is a consensus switching to Single is correct I agree with you @lucas34 that a breaking change makes most sense here, unfortunately.

@lucas34
Copy link
Contributor Author

lucas34 commented Apr 18, 2018

Do you have any branch yet for the RxSwift 5.0 to target the PR ?

@freak4pc
Copy link
Member

freak4pc commented Apr 8, 2019

Hey @lucas34 - We'd like to include this change in RxSwift 5, could you rebase on develop and finalize this in the next few days? Thanks !

@lucas34
Copy link
Contributor Author

lucas34 commented Apr 9, 2019

@freak4pc I created this MR to return Single instead of observable #1923

However I did with type conversions from observable to Single. Let me know what do you think. Not sure if it's the ideal solution.

@freak4pc
Copy link
Member

Fixed in #1923.

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