Implement Interop\Async\Promise #76
Conversation
| UnhandledRejectionException::nullOrResolve($reason), | ||
| null | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Both $onResolved calls should be wrapped with try / catch blocks and be forwarded to the error handler, currently the one of Loop, once the PR is merged, to the promise error handler: async-interop/promise#19
There was a problem hiding this comment.
Yes, but since we're requiring ^0.2.0 and do not require async-interop/event-loop there is no other way at the moment.
There was a problem hiding this comment.
I just fixed my patch, so we can merge the PR soon and have a new release hopefully.
| $onResolved( | ||
| UnhandledRejectionException::nullOrResolve($this->reason), | ||
| null | ||
| ); |
| if ($promiseOrValue instanceof AsyncInteropPromise) { | ||
| return new Promise(function ($resolve, $reject) use ($promiseOrValue) { | ||
| $promiseOrValue->when(function ($reason = null, $value = null) use ($resolve, $reject) { | ||
| $reject($reason ? $reason : $value); |
There was a problem hiding this comment.
I haven't found anything in the specification which defines how the callbacks must be invoked and if none, only $reason or both arguments are required.
There was a problem hiding this comment.
The specification states:
Any implementation MUST at least provide these two parameters.
But what I was rather asking here is whether $reject is fine with null being passed?
There was a problem hiding this comment.
There was a problem hiding this comment.
I just wasn't sure because of UnhandledRejectionException::nullOrResolve. RejectedPromise seems to miss a check for AsyncInteropPromise in __construct btw.
There was a problem hiding this comment.
I just wasn't sure because of UnhandledRejectionException::nullOrResolve.
nullOrResolve was actually wrong. In case of rejection without a reason, it would call the when() callbacks with null as first argument signalling success instead of failure. This is fixed in 8073b04.
RejectedPromise seems to miss a check for AsyncInteropPromise in __construct btw.
Good catch, fixed in 8c68ff6
| } | ||
|
|
||
| /** | ||
| * Implementations MAY fail upon resolution with an AsyncInteropPromise, |
There was a problem hiding this comment.
If they MAY fail, that means we can't rely on it?
There was a problem hiding this comment.
This comment is copied over from @bwoebi's https://github.com/async-interop/awaitable-test/blob/master/src/Test.php#L184 :)
There was a problem hiding this comment.
Oh, right. fail in the sense of being rejected.
|
Superseded by #78 |
Would be great if anyone from the async-interop team (@trowski, @kelunik, @bwoebi) could have a look at the implementation.
Closes #64