Implement AsyncInterop\Promise

[jsor]

Interoperability with async-interop/promise.

Closes #64

[trowski]

@jsor I fixed an issue with the @requires annotation in async-interop/promise-test. If you restart travis the tests should all pass this time.

[jsor]

@trowski Thanks 👍

[WyriHaximus]

FYI: this effects this issue async-interop/event-loop#128

[WyriHaximus]

Aside from my earlier comment about the possible namespace change

[jsor]

@WyriHaximus Upgraded to async-interop/promise ^0.4.0 with the new namespace.

[clue]

As much as I welcome interoperability with others, I'll have to vote 👎 on this for now.

My main concern here is how we now also offer a when() method while we currently focus on the then() method. If we were to drop our then() method this would make sense, but I don't really see this happen any time soon.

From a consumer perspective this is rather confusing and/or surprising, I wonder which method I should chose in which situation. At the very least, this would require some elaborate documentation and/or examples. The documentation currently links to and describes Promise/A+, but not the concept of having a when() method.

I have to admit my main concern should probably be directed at async-interop/promise and not this PR. I'll look into this in the upcoming days.

[jsor]

@clue I don't see this as an issue. I intentionally left out any documention of when() because i don't consider it part of the API of react/promise (while writing this, i realized 4b42c60 was wrong and reverted it). A consumer should not use when(), they should then() because that is our API.

when() is only for interoperabilty between libraries which use a placeholder for values which will become only available in the future, eg. by async operations.

The benefit is, that resolve() can properly turn return values from other libraries into react promises. Also, other libraries can use react promises in their way they handle interop promises, eg. through couroutines.

[WyriHaximus]

My main concern with not extending the interop promise that we can't communicate both interfaces that way. While all promise classes implement both interfaces, there isn't a single interface that communicates both interfaces. This is mainly coming from my work on @php-api-clients with PHP 7 return types.

For example the following function has a react promise return type hint as it can be a FulFilledPromise or a RejectedPromise. (Both of which implement ReactPromiseInterface and AsyncInteropPromise.)

function r(): ReactPromiseInterface

Now the following function only accepts a AsyncInteropPromise.

function g(AsyncInteropPromise $promise): AsyncInteropPromise

Technically g() should accept the result of r() when implementing both but it is imho not clearly communicating it will. When ReactPromiseInterface extends AsyncInteropPromise g() clearly communicates it can and will accept the result of r().

Now this all assumes PHP 7 (which I hope we'll move more towards this year (maybe even PHP 7 only release, but that's a different discussion)) return types and type hints. Having a single type you can hint makes it cleaner for end users IMHO compated to docblocks, or even IDE detection of possible return values.

---

From the work I've been doing over at @php-api-clients perspective depending on how we do the interop matters in user experience.

There are two options, extending would mean I can return ReactPromiseInterface which has our rich promise API with all it's functions. While also return AsyncInteropPromise through ReactPromiseInterface and have interoperability with other possible consumers.

While with implementing both ReactPromiseInterface and AsyncInteropPromise I would have either return ReactPromiseInterface or AsyncInteropPromise as return type. Hinting ReactPromiseInterface would expose our rich API, while AsyncInteropPromise would give interoperability but not our rich API. Now I could hint AsyncInteropPromise and wrap everything in a resolve(). That IMHO would clutter the examples and but more importantly when applying it on all levels make a unnecessary mess as all AsyncInteropPromise returning calls would have to be wrapped into resolve().

---

I agree with @jsor that when() is for interoperability and we should keep promising then() etc as our main API. How every I agree with @clue that this should be documentated very well.

Just my two euro's.

[trowski]

I agree with everything @WyriHaximus mentioned above. React's promises should extend the async-interop interface to make usability easier for everyone.

Linking to the async-interop standard should be sufficient to explain the inclusion of when() in React's promise interface. when() was designed to be lightweight, primarily for use with coroutines, not as a replacement for then() or done(). In fact, the name when() was chosen to avoid conflicts with the API of any of the existing promise implementations.

[jsor]

I disagree for the reason @clue is mentioning: confusion for the consumer.

My proposal here is, that the interop API is completely hidden from the consumer. They should just not use it. It is only intended for other libraries making it easier to consume react's promises through their resolve() counterpart.

Regarding @WyriHaximus example:

function g(AsyncInteropPromise $promise): AsyncInteropPromise

I think that this is just wrong. Why typehint against the placeholder rather the actual value the function is working on?

IMHO, the correct signature is

function g(MyValue $value): AsyncInteropPromise

And the function should be called like

React\Promise\resolve($promise)->done(function (MyValue $myValue) {
    g($myValue);
});

// or in a couroutine based library like amphp

/** @var $myValue MyValue **/
$myValue = yield $promise;
g($myValue);

[trowski]

The issue with React's promises not extending the async-interop interface is functions declaring React\Promise\PromiseInterface as a return type cannot automatically be assumed to have a when() method. If this cannot be relied on, we're right back where we started - wrapping one library's promises with another library's promises.

[kelunik]

I think that this is just wrong. Why typehint against the placeholder rather the actual value the function is working on?

I think @jsor is absolutely right there. But only as long as it's not some kind of combinator function. Combinator functions also benefit from interoperable promises, alongside with coroutines.

If ReactPromise doesn't extend InteropPromise, most return type declarations will declare ReactPromise I guess instead of InteropPromise. In that case, we can't be sure that ReactPromise also provides when().

If all of your own implementations implement both interfaces, it doesn't matter to them whether they do that or implement only ReactPromise which extends InteropPromise.

The only difference is that custom implementations of ReactPromise have to provide the interoperable when(), too.

For the consumer, nothing changes. All it changes is - as said - custom implementations. If you want to hide the when() method, fine, you could add an @internal annotation to it to discourage its use. But I honestly thing that this should be solved through documentation explaining what's when and what's then / done / otherwise

[jsor]

The issue with React's promises not extending the async-interop interface is functions declaring React\Promise\PromiseInterface as a return type cannot automatically be assumed to have a when() method.

I think this is a good thing because it prevents from mixing up async-interop promises with higher level placeholder abstractions like JS-style promises, coroutines, Rx obervables etc.. The async-interop interface just let you convert between these concepts.

If this cannot be relied on, we're right back where we started - wrapping one library's promises with another library's promises.

I think that's exactly not the purpose of the async-interop interface. Choose the promise/coroutine/rx/whatever-style library you prefer. If you use a dependency which prefers another style, the async-interop interface enables to convert from one style to another.

[kelunik]

I think that's exactly not the purpose of the async-interop interface. Choose the promise/coroutine/rx/whatever-style library you prefer. If you use a dependency which prefers another style, the async-interop interface enables to convert from one style to another.

That is exactly the aim of the interop spec. Being able to consume whatever implementation is used. Not adapting everything. If we want to adapt things, we don't need an interop spec, we can just write adapters for the most common libraries.

[kelunik]

Choose the promise/coroutine/rx/whatever-style library you prefer.

Right, that's what the interop spec is for. You can choose whatever you prefer, but you can always consume it via the interface defined in the interop spec. But that's only possible if ReactPromise extends InteropPromise.

Quoting from the current specification:

This specification is not designed to replace promise implementations that may be chained. Instead, the common interface may be extended by promise implementations.

[jsor]

Right, that's what the interop spec is for. You can choose whatever you prefer, but you can always consume it via the interface defined in the interop spec.

You could, but why would you? Why would you use when() directly and loose all the sugar react/promise, amphp etc. provides?

But that's only possible if ReactPromise extends InteropPromise.

Why that?

[bwoebi]

To be able to pass it directly into generic functionality … Coroutines, combinators etc. They all use the standards sole when() function.

[kelunik]

You could, but why would you? Why would you use when() directly and loose all the sugar react/promise, amphp etc. provides?

Amp doesn't provide any sugar on it's promise implementation. The sugar are coroutines and combinator functions, they directly consume any promise implementation using when.

So the application developer usually doesn't even call when. That's also the reason why we don't need any complex chainability.

react/promise might want to continue offering its then / done / otherwise API as an alternative to coroutines and thus might extend the InteropPromise. Application / library developers might then choose to use that instead of using coroutines.

Why that?

Because otherwise not every ReactPromise is a InteropPromise and thus not every ReactPromise can be consumed by other libraries such as the Amp combinators.

[WyriHaximus]

I think that this is just wrong. Why typehint against the placeholder rather the actual value the function is working on?

I think @jsor is absolutely right there. But only as long as it's not some kind of combinator function. Combinator functions also benefit from interoperable promises, alongside with coroutines.

There is no doubt that @jsor is right. That code snippet was merely to illustrate my point that when ReactPromiseInterface extending InteropPromise any other interop promise compatible package can consume them. Since ReactPromiseInterface is the contract represented by all concrete react promises it is very often used as (return) type hint (either through PHP 7 features or docblocks).

When ReactPromiseInterface extends InteropPromise all packages using react's promise package all of the sudden (well after an update) communicate they support interop promises without any changes to their code. (That is 146 dependents and anything depending on those. (Assuming they hint ReactPromiseInterface.))

[jsor]

My point is, that there's no such thing as "generic" :)

then() etc. is our API, when() is not and (most probably) won't be. I understand the amphp perspective where you're using a coroutine based style and where it's a lot easier to base the promise implementation on when() alone. And i'm perfectly fine with your interpretation of the async-interop interface, but i think we'll just have to agree to disagree here :)

I'm not completely against extending the async-interop interface but this might need some discussion with @reactphp/core.

[kelunik]

@jsor We tried repeatedly to get input from you guys, but nothing happened apart from @WyriHaximus participating and the some activity from you on the latest PRs. If you guys do not extend the interface now, then the promise interop project has failed.

then() etc. is our API, when() is not

Currently, because there's no reason to have when() currently. But interoperability brings that reason: interoperability.

I understand the amphp perspective where you're using a coroutine based style

The goal is enabling that to all promise implementations. So users might continue to use react/promise, but additionally use interoperable coroutines.

and where it's a lot easier to base the promise implementation on when() alone

Nobody suggests to drop then and use when alone. The suggestion is to support when additionally.

I'm not completely against extending the async-interop interface but this might need some discussion with @reactphp/core.

Please do so.

[stefanotorresi]

As a ReactPHP user, I'd love to see the this proposal implemented.
I hope @reactphp/core folks realize that ReactPHP is probably the biggest player in async PHP, and the successful adoption of a standard interface stands largely on their shoulders.
I realise you may have some design concerns (which tbh seem minor to me), but please weigh the advantages/disadvantages thoroughly.

[kelunik]

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."

[clue]

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 👍

[kelunik]

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.

[clue]

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 👍

[kelunik]

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.

[kelunik]

@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.

[jsor]

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().

[kelunik]

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

[jsor]

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.

[kelunik]

@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.

[kelunik]

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

[clue]

@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 :-)

[clue]

@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.

[kelunik]

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.

[clue]

@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 👍

[kelunik]

@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.

[clue]

"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).

[trowski]

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

[clue]

Thanks all for the discussion in this thread so far, I think this actually helped form and improve the current suggestion 👍

Also thanks @jsor, I think the current version with its documentation makes much more sense from a consumer perspective 👍

[clue]

@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 :-)

[clue]

@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.

[clue]

@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 👍

[clue]

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 👍

[clue]

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

[bwoebi]

@clue What's the current progress? Is this PR ready to be merged?
If you do, we also could look forward to officially release the 1.0.0 tag for AsyncInterop\Promise.

[clue]

Thanks @jsor, your suggested changes LGTM and there's currently nothing I'd ask you to change 👍 However, I'm not comfortable approving this yet and will leave my vote unchanged (for now) for the following reasons:

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.

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?

I understand my main concern should probably be directed at AsyncInterop instead, so I'm not sure it even makes sense to address this here.

This PR has received some interest among the people behind AsyncInterop, so perhaps you can also shed some light on this: What do you think about this PR in its current state?

[kelunik]

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?

Because it makes React compatible with coroutine implementations and generic combinator functions.

[trowski]

@clue By implementing AsyncInterop\Promise, react/promise is not endorsing the low-level API to it's consumers, rather acknowledging that a React promise may also be used anywhere that an interop promise can be used. These places will include, but not be limited to, coroutines, combinators, and (with support from Rx) observables.

When an API returns a React promise, a user should be encouraged to use the high-level API of the React promise. However, that same user can also know that they may use that promise in conjunction with any other library that accepts interop promises.

AsyncInterop\Promise only contains a low-level API because that is all that is necessary for interop. Any other promise methods can be implemented from the when() method. As a purely interop interface, we did not want to force a particular interface on promise libraries. What if the interop interface included methods that conflicted with those of React? We purposely avoided this problem with a low-level API consisting of a single, uniquely named method. Now this seems to be a blocker for adoption… to me this makes little sense.

Documentation of when() has been specifically excluded from this library, with which I take no issue. Use of when() should generally be discouraged by users. It exists for libraries, not applications. Amp discourages use of when() by encouraging users to use promises in coroutines. React discourages use of when() through their high-level then()/done() API. Right now it is difficult to intermix code from these libraries. With this PR, suddenly this will become trivial. React\Promise\resolve() could be used to transform an interop promise returned from Amp and use React's API. Conversely, a user of Amp could use a React promise directly in a coroutine.

Hopefully this will help you understand the purpose of this PR and the async-interop group. It is not to replace the API of any particular group, but rather allow sharing of code between libraries of different groups. We want to push async in PHP forward, making it as easy as possible to share code and collaborate, allowing each group to code to their own style and preferences, without having to worry about compatibility with a particular event loop or promise implementation.

[jsor]

Closing this for now. It might be reconsidered at a later point.