google / promises

Promises is a modern framework that provides a synchronization construct for Swift and Objective-C.
Apache License 2.0
3.8k stars 294 forks source link

[proposal] Add Guarantee or similar #108

Closed ghost closed 5 years ago

ghost commented 5 years ago

Hi,

Looks like its time to extend Promises, how about to add Guarantee, similar to example

very useful (in context of an app full of promises) when dealing with animations and delays, that can't fail

shoumikhin commented 5 years ago

Hi @umbri,

Could you provide some examples on how exactly having a Guarantee abstraction helps?

If I'm not mistaken, PromiseKit warns users to always chain something on top of the last then block (e.g. a catch handler, etc.), because then doesn't have a @discardableResult for a regular Promise class, but does have it for the Guarantee. Thus, if you want to avoid a warning that then result is unused, it's recommended to convert your Promise to a Guarantee and make it clear you don't want to chain anything else. The warning caused by the absence of @discardableResult is perceived as a benefit which forces users to chain catch at the end and is compared to Swift's do-try-catch language construct, where catch handler is mandatory.

To my mind, introducing a full-featured abstraction to the library with its own set of APIs and making it interchangeable with the regular Promise class in hope that users would use it to resolve the compiler warning which obligates them to utilize then result otherwise, sounds like an overkill that may bloat the APIs and complicate the library substantially.

I believe the promise error handling mechanism shouldn't be perceived as do-try-catch with a mandatory catch statement, but rather like a completion handler with the optional Value and Error arguments, where the error can be ignored entirely if users aren't interested in it, and simply unwrap the value only.

Consider:

func fetchValue<Value>(_ completion: (Value?, Error?) -> Void) { ... }

fetchValue { value, _ in
  if let value = value {
    print("The value is \(value)")
  }
}

Which becomes the following with Promises:

func fetchValue<Value>() -> Promise<Value> { ... }

fetchValue().then { value in
  print("The value is \(value)")
}

Therefore, if users don't want to catch anything and end the chain with then, it should be ok to do so.

Happy to hear other thoughts.

Thanks.

ghost commented 5 years ago

@shoumikhin Thank you for detailed response.

Some examples:

func after(_ delay: Double) -> Promise<Void> {
    return Promise { fulfill, _ in
        DispatchQueue.main.asyncAfter(deadline: .now() + delay, execute: {
            fulfill(())
        })
    }
}

extension UIView {
    func shake(duration: Double) -> Promise<Void> {
        return Promise(on: .main) { fulfill, _ in
            CATransaction.begin()
            CATransaction.setCompletionBlock {
                fulfill(())
            }

            // code ...

            CATransaction.commit()
        }
    }
}

Above can't fail! I understand that Error can be ignored but, I think an abstraction that will not have .catch() at all will be cleaner. Will be very useful when used inside Dependencies, you can clearly see that Promise can't fail ..

I believe the promise error handling mechanism shouldn't be perceived as do-try-catch with a mandatory catch statement, but rather like a completion handler with the optional Value and Error arguments, where the error can be ignored entirely if users aren't interested in it, and simply unwrap the value only.

I disagree with that, I think it is very important that user clearly know that he must catch errors. If we look at Swift syntax he is pushing us in the same direction.

P.S: Sorry about my English, not my native language

shoumikhin commented 5 years ago

So, in other words, you prefer to obligate the users to always chain a catch at the end of each promise chain, and in order to facilitate situations when such a mandatory catch is useless, you propose to introduce another promise class that doesn't require a catch?

Will be very useful when used inside Dependencies, you can clearly see that Promise can't fail ..

Do you mean that if we have a func that returns a Guarantee instead of a Promise, it explicitly tells us there's no need to catch anything?

I do agree that idea has its merit. Although, the implementation with an additional abstraction seems less than ideal and we barely can afford that without sacrificing simplicity. Beginning with the need to teach the users about the new class no one else ever heard of, except PromiseKit adopters, and best practices associated with it, and ending with a substantial refactoring followed by bugfix and unclear performance and memory footprint implications. Not sure the convenience of having a non-throwing promise variation is compelling enough to justify all the downsides.

BTW, there's an interesting analysis in Swift async/await proposal on how different languages and frameworks approach async error handling. Search for "make async be a subtype of throws instead of orthogonal to it". We have also chosen to make our Promise class throwable by default for the sake of simplicity, as of today, and the catch clause can be always ignored, when it makes sense.

ghost commented 5 years ago

Thank you for detailed response.

I has never used PromiseKit and has come to this working with my team day by day clearly seeing that, using Promises in our dependencies we need something like Guarantee

the implementation with an additional abstraction seems less than ideal and we barely can afford that without sacrificing simplicity

I am agree with above said

Not sure the convenience of having a non-throwing promise variation is compelling enough to justify all the downsides.

Same me, so why I have proposed this, may be there are other opinions ;)

BTW, there's an interesting analysis in Swift async/await proposal on how different languages and frameworks approach async error handling. Search for "make async be a subtype of throws instead of orthogonal to it". We have also chosen to make our Promise class throwable by default for the sake of simplicity, as of today, and the catch clause can be always ignored, when it makes sense.

Thank you for this, learned something new.

shoumikhin commented 5 years ago

has come to this working with my team day by day clearly seeing that, using Promises in our dependencies we need something like Guarantee

Can you elaborate a bit more on your use cases? Are you using a black box lib that provides some APIs returning promises, and you never know which of the promises can throw an error when subscribing to them, so have to add a catch clause every time? But if some of the APIs returned Guarantee instead, you’d know no catch is required? Is that your scenario?

Paraphrasing, are you using some 3rd party promises that you don’t know whether they can fail or not? Sounds like that, because if you knew every promise in your codebase that cannot fail, you’d quietly skipped the catch block for them with no issues.

ghost commented 5 years ago

Can you elaborate a bit more on your use cases? Are you using a black box lib that provides some APIs returning promises, and you never know which of the promises can throw an error when subscribing to them, so have to add a catch clause every time? But if some of the APIs returned Guarantee instead, you’d know no catch is required? Is that your scenario?

Yes! Thats it, we have "black boxes" (outsource), and components written in our company by some team, then used inside other components and so on ..., typically we are writing that Promise can't fail, but sometimes, .catch() dead code slips :)

Sounds like that, because if you knew every promise in your codebase that cannot fail, you’d quietly skipped the catch block for them with no issues.

We are doing this right now, but sometimes, especially with new team members, this don't work.

P.S: If Promises gets Guarantee I think that .then() block's@discardableResult can be removed so it shows warning if no .catch() was supplied

ghost commented 5 years ago

@umbri we would be open to reviewing a PR for adding Guarantee to Promises. Please feel free to submit a PR if you would like to help add that functionality. Thank you!