-
-
Notifications
You must be signed in to change notification settings - Fork 150
Implement Interop\Async\Promise #76
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,13 +2,15 @@ | |
|
|
||
| namespace React\Promise; | ||
|
|
||
| class RejectedPromise implements ExtendedPromiseInterface, CancellablePromiseInterface | ||
| use Interop\Async\Promise as AsyncInteropPromise; | ||
|
|
||
| class RejectedPromise implements ExtendedPromiseInterface, CancellablePromiseInterface, AsyncInteropPromise | ||
| { | ||
| private $reason; | ||
|
|
||
| public function __construct($reason = null) | ||
| { | ||
| if ($reason instanceof PromiseInterface) { | ||
| if ($reason instanceof PromiseInterface || $reason instanceof AsyncInteropPromise) { | ||
| throw new \InvalidArgumentException('You cannot create React\Promise\RejectedPromise with a promise. Use React\Promise\reject($promiseOrValue) instead.'); | ||
| } | ||
|
|
||
|
|
@@ -73,4 +75,12 @@ public function progress(callable $onProgress) | |
| public function cancel() | ||
| { | ||
| } | ||
|
|
||
| public function when(callable $onResolved) | ||
| { | ||
| $onResolved( | ||
| UnhandledRejectionException::resolve($this->reason), | ||
| null | ||
| ); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, needs a |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,12 +2,27 @@ | |
|
|
||
| namespace React\Promise; | ||
|
|
||
| use Interop\Async\Promise as AsyncInteropPromise; | ||
|
|
||
| function resolve($promiseOrValue = null) | ||
| { | ||
| if ($promiseOrValue instanceof ExtendedPromiseInterface) { | ||
| return $promiseOrValue; | ||
| } | ||
|
|
||
| if ($promiseOrValue instanceof AsyncInteropPromise) { | ||
| return new Promise(function ($resolve, $reject) use ($promiseOrValue) { | ||
| $promiseOrValue->when(function ($reason = null, $value = null) use ($resolve, $reject) { | ||
| if ($reason) { | ||
| $reject($reason); | ||
| return; | ||
| } | ||
|
|
||
| $resolve($value); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| if (method_exists($promiseOrValue, 'then')) { | ||
| $canceller = null; | ||
|
|
||
|
|
@@ -31,6 +46,14 @@ function reject($promiseOrValue = 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are both
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't found anything in the specification which defines how the callbacks must be invoked and if none, only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The specification states:
But what I was rather asking here is whether
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we allow rejection without a reason: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just wasn't sure because of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Good catch, fixed in 8c68ff6 |
||
| }); | ||
| }); | ||
| } | ||
|
|
||
| return new RejectedPromise($promiseOrValue); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| <?php | ||
|
|
||
| namespace React\Promise\PromiseTest; | ||
|
|
||
| trait AsyncInteropRejectedTestTrait | ||
| { | ||
| /** | ||
| * @return \React\Promise\PromiseAdapter\PromiseAdapterInterface | ||
| */ | ||
| abstract public function getPromiseTestAdapter(callable $canceller = null); | ||
|
|
||
| public function testWhenOnExceptionFailedAsyncInteropPromise() | ||
| { | ||
| $adapter = $this->getPromiseTestAdapter(); | ||
|
|
||
| $adapter->reject(new \RuntimeException); | ||
| $adapter->promise()->when(function ($e) use (&$invoked) { | ||
| $this->assertSame(get_class($e), "RuntimeException"); | ||
| $invoked = true; | ||
| }); | ||
| $this->assertTrue($invoked); | ||
| } | ||
|
|
||
| public function testWhenOnErrorFailedAsyncInteropPromise() | ||
| { | ||
| if (PHP_VERSION_ID < 70000) { | ||
| $this->markTestSkipped("Error only exists on PHP 7+"); | ||
| } | ||
|
|
||
| $adapter = $this->getPromiseTestAdapter(); | ||
| $adapter->reject(new \Error); | ||
| $adapter->promise()->when(function ($e) use (&$invoked) { | ||
| $this->assertSame(get_class($e), "Error"); | ||
| $invoked = true; | ||
| }); | ||
| $this->assertTrue($invoked); | ||
| } | ||
|
|
||
| public function testWhenOnExceptionFailedAsyncInteropPromiseWhenRejectedWithoutReason() | ||
| { | ||
| $adapter = $this->getPromiseTestAdapter(); | ||
|
|
||
| $adapter->reject(); | ||
| $adapter->promise()->when(function ($e, $value) use (&$invoked) { | ||
| $this->assertInstanceOf("Exception", $e); | ||
| $this->assertNull($value); | ||
| $invoked = true; | ||
| }); | ||
| $this->assertTrue($invoked); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both
$onResolvedcalls should be wrapped withtry/catchblocks and be forwarded to the error handler, currently the one ofLoop, once the PR is merged, to the promise error handler: async-interop/promise#19There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but since we're requiring
^0.2.0and do not requireasync-interop/event-loopthere is no other way at the moment.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just fixed my patch, so we can merge the PR soon and have a new release hopefully.