Add support for react promises in combinators and coroutines

[kelunik]

Don't know if we ever want to merge it.

[mention-bot]

By analyzing the blame information on this pull request, we identified @rdlowrey, @bwoebi and @Danack to be potential reviewers

[kelunik]

I'd prefer suggest here, as it's not a requirement, but then we can't have any version constraints.

[rdlowrey]

Agree with the suggest

[joshdifabio]

I think this might be better as if (method_exists($yielded, 'then')) {, which is what react and guzzle do for interoperability with other promise implementations. That way you don't need to worry about specific promise implementations. It's also what the JS libraries tend to do.

[joshdifabio]

I agree. It might also be faster not to create an amp promise but to instead simply to use the existing then method.

[joshdifabio]

Sorry, that last comment was supposed to be in response to your line comment about duplicating the if-statement (I'm replying from my phone).

[kelunik]

I don't think we'll merge this PR anyway. If we add support, that will be by providing another package.

I agree. It might also be faster not to create an amp promise but to instead simply to use the existing then method.

One could do that, but then you also have to duplicate all the $onResolve callbacks.

[joshdifabio]

I totally understand that you wouldn't want to add a dependency on react/promise, but what about simply duck-typing to support any thenable as I suggested previously? This is the approach React and Guzzle both take, as well as most JS promise libraries I believe, and would enable interoperability with React, Guzzle and Icicle promises. Guzzle and React's promise libraries have five million downloads each and masses of dependent libraries, surely it's worth supporting them?

One could do that, but then you also have to duplicate all the $onResolve callbacks.

Yeah, which would be a shame, but I thought in such a hot function you might prefer the more performant approach.

Edit: I should mention that I really love what you guys are doing, and I think your skill and the quality of your work is raising the bar for async libraries in PHP. The fact that your libraries are so good is the reason I want to be able to use them and that I'm so keen for them to be easy and convenient to work with. I'm really grateful for the effort you guys have put in. I also really appreciate your responsiveness.

[bwoebi]

That's doable too. But we probably need an event-loop adapter too. (Else you'll have to tick the loops in a busy main loop like each 50 milliseconds, which is not very nice.)

And at that point, the question really is, whether we shouldn't just introduce proper adapters?

[joshdifabio]

That's doable too. But we probably need an event-loop adapter too. (Else you'll have to tick the loops in a busy main loop like each 50 milliseconds, which is not very nice.)

guzzle/promises#27

If that PR is merged then I believe it'll be possible to run the Guzzle task queue (which is used to invoke promise observers) very efficiently, without the use of timers, and it looks like the same will be possible with react/promise v3. I really think promise interoperability is, and always should be, separate to event loop interoperability. Even if all my IO was handled by the Amp reactor, which is where I'd like to get to, I'd still have masses of code consuming and returning thenables.

[rdlowrey]

I know I've been absentee for a while but I might be persuaded to incorporate promise compat separate from event loop compat. It's at least worth considering. Even though I personally think the amp solutions are better (that's why we wrote them that way, after all), we're unlikely to gain wide adoption without finding ways to integrate with these other libs.

It's something to consider.

[bwoebi]

@joshdifabio I'm not sure what you're talking about here? The main issue is waiting for I/O.

Typically an event reactor is blocking inside one stream_select() call. That stream_select() call has to know about all the streams we're waiting for.
To allow for another reactors events to be dispatched, you have this reactor to run his stream_select() call with his streams.
The only solutions to this are really calling the other reactors tick function at regular intervals or dispatching the event listeners to our reactor via an adapter.

[joshdifabio]

@joshdifabio I'm not sure what you're talking about here? The main issue is waiting for I/O.

I'm talking about working with a single event loop. React promises and Guzzle promises have precisely zero dependencies on any event loop. It's entirely possible to run only an Amp reactor and still have multiple promise implementations in your application. Does that make sense? Promise interoperability is not dependent on event loop interoperability.

Edit: I explained my situation in detail previously. Maybe if you read that it'll be clearer. Just to really emphasise it: I do not plan to run multiple event loops, but I still use multiple promise implementations.

[kelunik]

but what about simply duck-typing to support any thenable as I suggested previously?

That might break other objects having a then method not being a thenable. Technically, even adding the ReactPromise is a BC break.

React promises and Guzzle promises have precisely zero dependencies on any event loop.

The promises don't have that dependency, but the libraries usually have.

Guzzle requires being called in every event loop tick, see https://github.com/guzzle/promises/blob/8c54fb01b3b077f07f74f658eac1c558930cfbc3/README.md#event-loop-integration. That's exactly the busy loop we're talking about.

[joshdifabio]

That might break other objects having a then method not being a thenable. Technically, even adding the ReactPromise is a BC break.

Absolutely any change can be a BC break. When you fix a bug you are changing functionality which people might have been relying on. Sometimes a little pragmatism is helpful.

Guzzle requires being called in every event loop tick, see https://github.com/guzzle/promises/blob/8c54fb01b3b077f07f74f658eac1c558930cfbc3/README.md#event-loop-integration. That's exactly the busy loop we're talking about.

Which is exactly why I submitted the PR I referenced above, which removes this requirement. If nothing has been added to the task queue then there is no need to run it.

I don't think there's really any more I can say on this, it's up to you guys what you want to do. Thanks for hearing me out.