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

Implement AsyncInterop\Promise #78

Closed
wants to merge 14 commits into from
Closed
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ Table of Contents
* [map()](#map)
* [reduce()](#reduce)
* [PromisorInterface](#promisorinterface)
* [AsyncInterop\Promise compatibility](#asyncinteroppromise-compatibility)
4. [Examples](#examples)
* [How to use Deferred](#how-to-use-deferred)
* [How promise forwarding works](#how-promise-forwarding-works)
Expand Down Expand Up @@ -151,6 +152,11 @@ and an associated value, or rejection (failure) and an associated reason.
Once in the fulfilled or rejected state, a promise becomes immutable.
Neither its state nor its result (or error) can be modified.

This interface extends the
[`AsyncInterop\Promise`](https://github.com/async-interop/promise) interface.
See [`AsyncInterop\Promise` compatibility](#asyncinteroppromise-compatibility),
for further information.

#### Implementations

* [Promise](#promise-1)
Expand Down Expand Up @@ -402,6 +408,10 @@ Creates a promise for the supplied `$promiseOrValue`.
If `$promiseOrValue` is a value, it will be the resolution value of the
returned promise.

If `$promiseOrValue` is a
[`AsyncInterop\Promise`](https://github.com/async-interop/promise), a trusted
promise that follows the state of the `AsyncInterop\Promise` is returned.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a very simple example how we can consume an AsyncInteropPromise? Such as passing this through the resolve() function etc.?


If `$promiseOrValue` is a thenable (any object that provides a `then()` method),
a trusted promise that follows the state of the thenable is returned.

Expand Down Expand Up @@ -512,6 +522,27 @@ The `React\Promise\PromisorInterface` provides a common interface for objects
that provide a promise. `React\Promise\Deferred` implements it, but since it
is part of the public API anyone can implement it.

### AsyncInterop\Promise compatibility

The [`PromiseInterface`](#promiseinterface) extends the
[`AsyncInterop\Promise`](https://github.com/async-interop/promise) interface and
thereby all promise implementations from React/Promise implement its `when()`
method.

The `AsyncInterop\Promise` interface is intended for interoperability with other
libraries and their combinator and conversion functions.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: "The AsyncInterop\Promise interface allows interoperability with other promise implementations and their combinator and coroutine implementations."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kelunik While I understand your point, I actually prefer the current version (which is slightly more conservative and less enthusiastic). After all, while we all aim for this, "allows interoperability" has yet to be proven :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsor Can we add a very simple example to show how this interoperability may look like from a consumer perspective? Such as a dummy combinator function with the proper typehint etc.?

See also above for the documentation and/or example on how we can consume an AsyncInteropPromise.


Note, that although the interface is named *Promise*, it has nothing to do with
promises commonly known from the
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd reword this paragraph. While the interop promise may have nothing to do with the concrete interface of A+, it's still the same thing. A placeholder representing the result of an async operation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kelunik While I understand your point, I actually prefer the current version from a consumer perspective who is familiar with the term "promise" from the other projects enlisted below. I like the "A placeholder representing the result of an async operation." though 👍

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clue I understand your point, but I'm not sure whether it really helps consumers to understand it. They'll just think "Why the hell is it called promise if it's something entirely different?!" and be more confused than before. We had a really long discussion about the naming in async-interop/promise#4 and another one to switch back to Promise, but I can't find that one right now. The reason behind it is that we kept calling it Promise while talking about it, because it's just that, just with a different consumer API.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Why the hell is it called promise if it's something entirely different?!"

I think this is a very important point here: To me, this whole PR seems to convey the notion that this library implements a concept commonly known as "promises", while async-interop/promise only offer a low-level API that aims to ease interoperability between different promise vendors.

We had a really long discussion about the naming in async-interop/promise#4 and another one to switch back to Promise, but I can't find that one right now.

This isn't exactly transparent to me either, so I'd very much appreciate if you happen to find this and could link to this here 👍

[…] we kept calling it Promise while talking about it, because it's just that, just with a different consumer API.

I understand the reasoning – but would disagree from a consumer perspective. If there's project A (e.g. this project) that describes "promises" and the interop standard that describes a different API then it's not the same from a consumer perspective.

This means that project A should either ditch its description and adopt the interop API or the other way around (which I don't see happening any time soon).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clue @kelunik Here is the related PR changing the name back to Promise: async-interop/promise#15

[CommonJS Promises/A](http://wiki.commonjs.org/wiki/Promises/A),
[Promises/A+](https://promisesaplus.com) or
[ECMAScript 2015 (ES6)](http://www.ecma-international.org/ecma-262/6.0/#sec-promise-constructor)
specifications.

If you're using React/Promise as your primary value placeholder implementation,
it is recommended to **not** use the `when()` method directly but the methods
provided by the [`PromiseInterface`](#promiseinterface).
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be reworded to: "If you use react/promise implementations, it's recommended to use the high level consumption methods then(), done(), otherwise() and always() to consume a promise's value or reason."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a consumer perspective, I actually prefer the explicit recommendation against when() because this would otherwise leave me wondering why it exists in the first place. However, I also like your suggestion of explicitly pointing the user to the correct alternatives 👍

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, doesn't the explicit recommendation against it make you wonder even more why it exists? The alternatives are in no way more "correct" than when(), they're just more high-level, and that's how I'd personally word it. You can totally use when(), but be aware of it being low-level. I think it makes sense to mention a reason when to use when(): It makes sense whenever your function accepts promises instead of direct values, as it can accept any promise then from any vendor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for my wording, "correct" wasn't intended to mean the alternative is "incorrect", but rather "not recommended and/or endorsed".

You can totally use when(), but be aware of it being low-level.

I guess this is the culprit here: Why would react/promise endorse a low-level API to its consumers if it offers a (conceived or otherwise) more powerful and comfortable higher-level API? See also the comments below 👍

Copy link

@kelunik kelunik Jan 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It totally depends on what's consumer for you. Is it just application developers or also library developers? The latter will probably want to use when() if they also accept other promise implementations instead of wrapping all promises in React's promises. If all you need is a simple callback on completion (no chains), when() is pretty equivalent to always() / done() (depending whether you need the value / reason or not), except that it works on all compatible implementations then, not only React.

In fact, you might want to adapt done() to when() semantics regarding the error handling to provide a clear separation of concerns. IMO, throwing from resolve / reject because the consumption of the value failed is wrong. @jsor mentioned that in our face2face meeting as one of the ugliest things in the current promise implementation.

<?php

require __DIR__ . "/vendor/autoload.php";

$deferred = new React\Promise\Deferred;

$deferred->promise()->done(function ($value) {
    var_dump("value", $value);
}, function ($reason) {
    throw new Exception;
    var_dump("reason", $reason);
});

try {
    // shouldn't throw here, instead forward to the interop error handler
    $deferred->reject("foobar");
} catch (Exception $e) {
    var_dump("catch", $e);
}

Sorry for the lengthy discussion here, should I open a separate issue for that one? It depends on this PR, but I think it could make sense for 3.0.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsor You're right about always, thought it would only allow clean ups, missed that it supports forwarding as well.

My main point is only about this statement from your README:

Calling done() transfers all responsibility for errors to your code. If an error (either a thrown exception or returned rejection) escapes the $onFulfilled or $onRejected callbacks you provide to done, it will be rethrown in an uncatchable way causing a fatal error.

Because in fact, those errors aren't uncatchable, they bubble up to $deferred->resolve() and then further up from there. To prevent exactly the bubbling up to $deferred->resolve(), the interop spec introduced the ErrorHandler which can either log such an error gracefully or abort the script if not such handler is defined.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because in fact, those errors aren't uncatchable, they bubble up to $deferred->resolve() and then further up from there.

Yes, they are catchable if resolution happens synchronously. catchable is a bit misleading here as it means uncatchable by any handler, not (only) from try/catch.

the interop spec introduced the ErrorHandler which can either log such an error gracefully or abort the script if not such handler is defined.

Nothing prevents you from transferring the error to the ErrorHandler from inside done().

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has really become quite lengthy now. Want to continue in chat?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might hop in when time allows. But, we're not going to change the behavior of done(). It's a defacto standard among promise libraries and it's is in line with all implementations i'm aware of.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsor Bluebird for one doesn't let errors from done bubble up to the resolve call, it executes async.fatalError and thus exists. If it's not in node, it doesn't let it bubble up either, it defers it to the next tick and throws from a 0 timeout callback, similar what we had before we had the error handler. But as we don't have the a loop in the language, we wanted to decouple those and introduced a separate error handler.


Examples
--------

Expand Down
4 changes: 3 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@
{"name": "Jan Sorgalla", "email": "[email protected]"}
],
"require": {
"php": ">=5.4.0"
"php": ">=5.4.0",
"async-interop/promise": "^0.4.0"
},
"require-dev": {
"async-interop/promise-test": "^0.4.1",
"phpunit/phpunit": "~4.8"
},
"autoload": {
Expand Down
15 changes: 14 additions & 1 deletion src/FulfilledPromise.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@

namespace React\Promise;

use AsyncInterop\Promise as AsyncInteropPromise;

final class FulfilledPromise implements PromiseInterface
{
private $value;

public function __construct($value = null)
{
if ($value instanceof PromiseInterface) {
if ($value instanceof AsyncInteropPromise) {
throw new \InvalidArgumentException('You cannot create React\Promise\FulfilledPromise with a promise. Use React\Promise\resolve($promiseOrValue) instead.');
}

Expand Down Expand Up @@ -66,4 +68,15 @@ public function always(callable $onFulfilledOrRejected)
public function cancel()
{
}

public function when(callable $onResolved)
{
try {
$onResolved(null, $this->value);
} catch (\Throwable $exception) {
AsyncInteropPromise\ErrorHandler::notify($exception);
} catch (\Exception $exception) {
AsyncInteropPromise\ErrorHandler::notify($exception);
}
}
}
5 changes: 5 additions & 0 deletions src/LazyPromise.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ public function cancel()
return $this->promise()->cancel();
}

public function when(callable $onResolved)
{
return $this->promise()->when($onResolved);
}

/**
* @internal
* @see Promise::settle()
Expand Down
11 changes: 11 additions & 0 deletions src/Promise.php
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,17 @@ public function cancel()
$this->call($canceller);
}

public function when(callable $onResolved)
{
if (null !== $this->result) {
return $this->result()->when($onResolved);
}

$this->handlers[] = function (PromiseInterface $promise) use ($onResolved) {
$promise->when($onResolved);
};
}

private function resolver(callable $onFulfilled = null, callable $onRejected = null)
{
return function ($resolve, $reject) use ($onFulfilled, $onRejected) {
Expand Down
4 changes: 3 additions & 1 deletion src/PromiseInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

namespace React\Promise;

interface PromiseInterface
use AsyncInterop\Promise as AsyncInteropPromise;

interface PromiseInterface extends AsyncInteropPromise
{
/**
* @return PromiseInterface
Expand Down
18 changes: 17 additions & 1 deletion src/RejectedPromise.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@

namespace React\Promise;

use AsyncInterop\Promise as AsyncInteropPromise;

final class RejectedPromise implements PromiseInterface
{
private $reason;

public function __construct($reason = null)
{
if ($reason instanceof PromiseInterface) {
if ($reason instanceof AsyncInteropPromise) {
throw new \InvalidArgumentException('You cannot create React\Promise\RejectedPromise with a promise. Use React\Promise\reject($promiseOrValue) instead.');
}

Expand Down Expand Up @@ -74,4 +76,18 @@ public function always(callable $onFulfilledOrRejected)
public function cancel()
{
}

public function when(callable $onResolved)
{
try {
$onResolved(
UnhandledRejectionException::resolve($this->reason),
null
);
} catch (\Throwable $exception) {
AsyncInteropPromise\ErrorHandler::notify($exception);
} catch (\Exception $exception) {
AsyncInteropPromise\ErrorHandler::notify($exception);
}
}
}
23 changes: 23 additions & 0 deletions src/functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,27 @@

namespace React\Promise;

use AsyncInterop\Promise as AsyncInteropPromise;

function resolve($promiseOrValue = null)
{
if ($promiseOrValue instanceof PromiseInterface) {
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;

Expand All @@ -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);
});
});
}

return new RejectedPromise($promiseOrValue);
}

Expand Down
20 changes: 20 additions & 0 deletions tests/AsyncInterop/DeferredTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<?php

namespace React\Promise\AsyncInterop;

use AsyncInterop\Promise\Test;
use React\Promise\Deferred;

class DeferredTest extends Test
{
public function promise(callable $canceller = null)
{
$d = new Deferred($canceller);

return [
$d->promise(),
[$d, 'resolve'],
[$d, 'reject']
];
}
}
25 changes: 25 additions & 0 deletions tests/AsyncInterop/LazyPromiseTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?php

namespace React\Promise\AsyncInterop;

use AsyncInterop\Promise\Test;
use React\Promise\Deferred;
use React\Promise\LazyPromise;

class LazyPromiseTest extends Test
{
public function promise(callable $canceller = null)
{
$d = new Deferred($canceller);

$factory = function () use ($d) {
return $d->promise();
};

return [
new LazyPromise($factory),
[$d, 'resolve'],
[$d, 'reject']
];
}
}
25 changes: 25 additions & 0 deletions tests/AsyncInterop/PromiseTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?php

namespace React\Promise\AsyncInterop;

use AsyncInterop\Promise\Test;
use React\Promise\Promise;

class PromiseTest extends Test
{
public function promise(callable $canceller = null)
{
$resolveCallback = $rejectCallback = null;

$promise = new Promise(function ($resolve, $reject) use (&$resolveCallback, &$rejectCallback) {
$resolveCallback = $resolve;
$rejectCallback = $reject;
}, $canceller);

return [
$promise,
$resolveCallback,
$rejectCallback
];
}
}
8 changes: 8 additions & 0 deletions tests/FulfilledPromiseTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,12 @@ public function shouldThrowExceptionIfConstructedWithAPromise()

return new FulfilledPromise(new FulfilledPromise());
}

/** @test */
public function shouldThrowExceptionIfConstructedWithAAsyncInteropPromise()
{
$this->setExpectedException('\InvalidArgumentException');

return new FulfilledPromise(new SimpleFulfilledAsyncInteropTestPromise());
}
}
37 changes: 37 additions & 0 deletions tests/FunctionRejectTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,24 @@ public function shouldRejectAFulfilledPromise()
);
}

/** @test */
public function shouldRejectAFulfilledAsyncInteropPromise()
{
$resolved = new SimpleFulfilledAsyncInteropTestPromise();

$mock = $this->createCallableMock();
$mock
->expects($this->once())
->method('__invoke')
->with($this->identicalTo('foo'));

reject($resolved)
->then(
$this->expectCallableNever(),
$mock
);
}

/** @test */
public function shouldRejectARejectedPromise()
{
Expand All @@ -61,4 +79,23 @@ public function shouldRejectARejectedPromise()
$mock
);
}

/** @test */
public function shouldRejectARejectedAsyncInteropPromise()
{
$exception = new \Exception('foo');
$resolved = new SimpleRejectedAsyncInteropTestPromise($exception);

$mock = $this->createCallableMock();
$mock
->expects($this->once())
->method('__invoke')
->with($this->identicalTo($exception));

reject($resolved)
->then(
$this->expectCallableNever(),
$mock
);
}
}
Loading