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

Making Stream error generic #32

Closed
DarthMike opened this issue Apr 30, 2015 · 5 comments
Closed

Making Stream error generic #32

DarthMike opened this issue Apr 30, 2015 · 5 comments

Comments

@DarthMike
Copy link

Currently Streams supply generic values, but the error is cocoa NSError.

I think it would be more future-proof and user friendly to support generic errors. Many times when dealing with Swift codes, the errors are simple enums.

What do you think?

@inamiy
Copy link
Member

inamiy commented May 1, 2015

Yes, I basically agree with you.
There are however quite many places needed to fix (especially when cancel(error: nil) occurs), so let me take this into consideration.

@inamiy
Copy link
Member

inamiy commented May 1, 2015

Just wrote a short sketch in 3b0942a (generic-error-type branch).

It works pretty well, but there are also some penalties 😓

  • lengthy class name (harder to read code)
  • unavailability of type inference for some methods (e.g. Stream<Int, MyError>.sequence([1, 2, 3]))
  • conversion method (from Stream<T, E1> to Stream<T, E2> will be required, let's say mapError(f: Error1 -> Error2), but many users might find this cumbersome to use

I'm not sure how much benefit can be earned by using generic error type,
but just like in other language (e.g. RxJava, Rx.NET) using a basic Exception class,
I thought ReactKit should keep using NSError just because it heavily relies on Foundation.framework.

@DarthMike
Copy link
Author

I just find more and more that I don't like to use NSError when dealing with pure Swift code. So I thought it would be good to have errors as enums.
Why would we need to map errors? If the constraint is conforming to ErrorType, two different enums would be equally conformant to the generic constraints?

Also I'm not sure why cancellation is treated as error. Maybe not erroring when cancelled would simplify logic?

@inamiy
Copy link
Member

inamiy commented May 4, 2015

Why would we need to map errors?

mapError() will be required when you want to combine multiple streams into one.
For example,

let stream1: Stream<Int, MyEnumError> = ...
let stream2: Stream<Int, NSError> = ...

// ERROR: `stream1` & `stream2` must be same type for using `zip()`
let zippedStream = stream1 |> zip (stream2) // compile error

// This is OK
let stream2b: Stream<Int, MyEnumError> = stream2 |> mapError(... /* map NSError to MyEnumError */ )
let fixedZippedStream = stream1 |> zip (stream2b) // OK

This is not just a NSError issue. If you want to combine your Stream<Int, MyEnumError> with other 3rd party provided Stream<Int, YetAnotherEnumError>, you will always require mapError().

Also I'm not sure why cancellation is treated as error.

This is part of SwiftTask design, where rejection (must send Error) occurs internally while cancellation (sending Error is not necessary) takes place externally.
In ReactKit, when upstream is cancelled without attaching error object, it requires some other Error instance to internally notify its end to downstream, so there needs a "cancel -> internal error" conversion logic as I added its protocol in Error.swift#L13-L16.

Maybe not erroring when cancelled would simplify logic?

Above mentioned can be avoided by specifying generic ReactKit.Error type as Optional (sending nil instead of cumbersome MyEnumError.cancelledError()), but it might cause some areas required to handle double-Optional (i.e. error: ErrorType??) which is yuck.
So, I need a bit more time to take this idea into consideration...

@inamiy
Copy link
Member

inamiy commented Sep 23, 2015

I just bumped ver 0.12.0 and decided to replace NSError to ErrorType.
Please see https://github.com/ReactKit/ReactKit/releases/tag/0.12.0 for more detail.

@inamiy inamiy closed this as completed Sep 23, 2015
@inamiy inamiy added this to the 0.12.0 milestone Sep 23, 2015
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

2 participants