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

Use new Single unit from RxSwift 3.3 #77

Closed
joshfriend opened this issue Apr 5, 2017 · 10 comments
Closed

Use new Single unit from RxSwift 3.3 #77

joshfriend opened this issue Apr 5, 2017 · 10 comments

Comments

@joshfriend
Copy link

RxSwift 3.3 added a new Single trait (formerly called "unit"):

A Single is a variation of Observable that, instead of emitting a series of elements, is always guaranteed to emit either a single element or an error.

Docs

As mentioned in RxSwift docs, HTTP requests fit this model extremely well.

@joshfriend joshfriend changed the title Use new Single unit Use new Single unit from RxSwift 3.3 Apr 5, 2017
@freak4pc
Copy link
Member

freak4pc commented Apr 5, 2017

Sounds like a good idea. Only situation to consider is if you'd want to have a replay behaviour but in that case you can always do asObservable, so I assume it would fit more cases in general.

Another issues with this is it would probably have to be an additional/alternative implementation and not a substitution to avoid breaking changes to people using the Observable variant.

@joshfriend
Copy link
Author

What do you think about this strategy?

  • Convert existing API to returning Single
  • Shim the Single returning APIs to return Observable to keep existing API surface intact
  • Marking Observable returning shim APIs as deprecated
  • Remove Observable shims in a later release

@freak4pc
Copy link
Member

freak4pc commented Apr 5, 2017

My thought was the first 2, but is there a good reason to remove Observable(s) at all? If someone wants them he can just use them.

@sergdort
Copy link
Member

sergdort commented Apr 5, 2017

Also don't forget that not all HTTP requests have single response. there is also text/event-stream

@joshfriend
Copy link
Author

is there a good reason to remove Observable(s) at all?

My thought was that if it's already easy enough to call asObservable on a Single, removing those Observable APIs would make it easier to maintain this library in the future since you wouldn't have to maintain essentially 2 nearly identical copies of most API functions. I suppose the same thing could also apply to calling asSingle on an Observable, but if the Single trait makes the most sense for an HTTP call (in many cases), then that one should be the "main" API?

not all HTTP requests have single response.

👍 Good note, I'll keep that in mind if I end up trying to work on this.

@freak4pc
Copy link
Member

freak4pc commented Apr 5, 2017

If you don't end up working on this LMK, I wouldn't mind handling as well :) @joshfriend

Good luck!

@joshfriend
Copy link
Author

I came to an unfortunate realization last night. Swift allows overloading methods that differ only in return type:

func data() -> Observable<Data>
func data() -> Single<Data>

However, when you try to use one of those functions, you get a compile error "Type of expression is ambiguous without more context" or something similar. You need to cast or hint the type of the desired return value somehow:

let request = data() as Single<Data>
// or
let request: Single<Data> = data()

Thus, adding a parallel APIs returning Single (that use the same method names) is a breaking change, even though the Observable returning APIs were unchanged. Adding those casts everywhere is also very ugly.

Possible workaround would be to name the functions differently for the Single APIs

// Current API:
func data() -> Observable<Data>

// New APIs:
func getData() -> Single<Data>
// or
func dataSingle() -> Single<Data>

I don't really like those names though, the existing API names are quite good.

@freak4pc
Copy link
Member

freak4pc commented Apr 6, 2017

@joshfriend Yeah I kinda had that thought yesterday as well. This is kind of a problem, I don't have a good thought about it yet but I have a couple of questions we need to answer :

  • Will we eventually want to make Single<T> the main response type instead of Observable<T> ?
  • If so, when? This would be a breaking change probably so it needs to be on a major version.
  • Also, should we rename the old Observable implementation to observableData(), for example? Since it is the way-less-commonly-used scenario ? (e.g. text/stream like @sergdort mentioned)
  • And lastly, does the Shimming strategy actually fit here? If you do a .asObservable() on aSingle you can manipulate it as an Observable but you would still only get a single emission if I'm not mistaken, which might mean a Shim wont be good here and we're better off keeping separate strategies or using the shim the other way (e.g. asSingle()) ?

All good answers we should answer before we proceed here I imagine.

Would love everyone's thoughts :)

@kzaher
Copy link
Member

kzaher commented Apr 8, 2017

Hi guys,

I think the simplest solution would be to create a new major version of RxAlamofire that uses Single when appropriate.

@freak4pc
Copy link
Member

freak4pc commented May 4, 2017

Marking this as enhancement for now but I still think like this would need some more thought to make sure API still feels consistent and doesn't break too many things :)

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

4 participants