PromiseKit icon indicating copy to clipboard operation
PromiseKit copied to clipboard

Using PMKError.cancelled unexpectedly (to a newbie) stops chain from completing. Document somehow??

Open ConfusedVorlon opened this issue 4 years ago • 8 comments

  • Please specify the PromiseKit major version you are using

7.0.0.rc2

installed with SPM

I created a promise wrapper for NSAlert. It seemed sensible to use seal.reject(PMKError.cancelled) in the case where the user clicked 'cancel'

I didn't realise that this error has a special-case treatment where the .catch block isn't called.

Presumably there isn't a good 'code' way to warn about this - but it could be flagged in the troubleshooting section under 'My promise never resolves'

Example below. To me, it was very surprising that neither the .done nor the .catch block were called

       let promise:Promise<Void> = Promise {
           seal in
           
           seal.reject(PMKError.cancelled)
       }
       
       firstly {
           promise
       }
       .done {
           print("done")
       }
       .catch {_ in
           print("Catch")
       }

ConfusedVorlon avatar Sep 21 '21 13:09 ConfusedVorlon

Documented: https://github.com/mxcl/PromiseKit/blob/v6/Documentation/CommonPatterns.md#cancellation Documented: https://github.com/mxcl/PromiseKit/blob/v6/Sources/Catchable.swift#L18

mxcl avatar Sep 21 '21 13:09 mxcl

v7 docs seem to have both made cancelization incredibly complex and also made the docs unreadable. So, yeah this could be improved.

Sadly I don't really understand how cancellation works anymore. @dougzilla32 can maybe explain.

Whatever the cancellation documentation is now intimidating and does not actually emphasize the fact canceled errors are by default not caught. Which IMO is a serious step backwards in our quality.

mxcl avatar Sep 21 '21 13:09 mxcl

Example below. To me, it was very surprising that neither the .done nor the .catch block were called

Cancelation is neither success nor failure. Hence this behavior. Instead use a finally if you need to do some clean up. This is a core tenant of our design which apparently is no longer really mentioned.

Considering I don't really understand how it works I'm not sure we should recommend v7 at this point.

Maintaining this library is not well paid (~1K a year in donations), so considering the thousands of hours I have spent on it previously, it is hard to justify time on it nowadays that I have a family to take care of and my time should be paid.

mxcl avatar Sep 21 '21 13:09 mxcl

understood - and thanks for your work!

re cancellation, perhaps one for @dougzilla32 - could PromiseKit print a debug error if a promise isn't explicitly cancellable, but returns PMKError.cancelled ?

ConfusedVorlon avatar Sep 21 '21 15:09 ConfusedVorlon

ok - just tested with 6.15.3 and the behaviour is the same. I'll reopen my doc pull requests!

ConfusedVorlon avatar Sep 21 '21 15:09 ConfusedVorlon

Yeah we really should print a debug message, this catches people, no pun intended.

mxcl avatar Sep 23 '21 11:09 mxcl