PromiseInterface: add `@yield` annotation #536

roxblnfk · 2023-09-13T19:28:11Z

roxblnfk
Sep 13, 2023

I often use React Promises in coroutines. It's hard without type hints in the IDE when a result comes from a yield $promise.

We can fix this with the @yield annotation. Adding @yield T to the PromiseInterface lets us set the return type for static analyzers and the IDE.

I don't know if PhpStan supports this, I only found a feature request here: phpstan/phpstan#4245.
But, Psalm and PhpStorm support it.

I use VirtualPromise, which extends the PromiseInterface, just for typing in annotations.

/**
 * @template T
 * @yield T
 * @extends \React\Promise\PromiseInterface<T>
 */
interface VirtualPromise extends \React\Promise\PromiseInterface
{
}

PR: reactphp/promise#254

yadaiio · 2023-10-24T15:36:44Z

yadaiio
Oct 24, 2023

Hey @roxblnfk, thank you for bringing this up.

@WyriHaximus is currently working on ticket reactphp/async#40, where he will define template types for the coroutine function. By the way, if you happen to use async in version 4.x I would recommend to use Fibers instead of coroutines.

With that in mind I will ping @SimonFrings to go ahead and close reactphp/promise#254 as @WyriHaximus is working on reactphp/async#40.

5 replies

roxblnfk Oct 24, 2023
Author

Hi 👋

In the Temporal SDK, we don't use reactphp/async. Instead, we have our own implementation of coroutines.
In our case, the user simply yields a promises out of the generator to get the await behavior.

However, I don't understand how the change I proposed affects @WyriHaximus's work and vice versa. Could you explain?

WyriHaximus Oct 24, 2023
Maintainer

@yadaiio @SimonFrings I'm not 100% sure closing reactphp/promise#254 is the right cause of action here. While I don't think @roxblnfk's PR's adds something we'll use directly in our core component, I'm not against adding something that benefits the wider community. Also I'm not sure if there is any overlap between reactphp/promise#254 and reactphp/async#40. Which has of course to be validated, plus we need to validate reactphp/promise#254 doesn't cause any issues with our template vars across packages. But again, I'm not against adding it.

SimonFrings Oct 27, 2023
Maintainer

Well, first of all I can say that @yadaiio is not wrong here...

In the Temporal SDK, we don't use reactphp/async. Instead, we have our own implementation of coroutines.

Initially, I assumed you were using the React\Async\coroutine function. However, if you are using your own implementation, you need to fix this issue for your own coroutine() function.

I'm not 100% sure closing reactphp/promise#254 is the right cause of action here

After reviewing reactphp/promise#254, I couldn't find any confirmation that the suggested changes have been tested and produce the desired results. Moreover, @clue showed an example where merging reactphp/promise#254 also has a negative impact, but I can't go much into detail here, so let me summon @clue for an better explanation. 😄

@clue dedicated some time to reactphp/async#40 and we also had a call with @WyriHaximus today about these topics. It appears that PHPStan doesn't fully support the yield statement, @clue is planning to open a new ticket for this in PHPStan.

In my opinion, with the introduction of Fibers, coroutines have become somewhat outdated as you can easily use the async() and await() functions instead. The most important functions (async() and await()) are already working well, so I believe we can merge reactphp/async#40 without waiting for PHPStan to support the yield annotation (after applying our discussed changes).

roxblnfk Oct 27, 2023
Author

However, if you are using your own implementation, you need to fix this issue for your own coroutine() function.

This issue concerns only users, not our SDK code.
Without the @yield annotation, their Workflow will lack typing and IDE hints.

After reviewing reactphp/promise#254, I couldn't find any confirmation that the suggested changes have been tested and produce the desired results

I wrote about that in the first message of the discussion ¯\_(ツ)_/¯
There is just a screenshot but I can make a Gist for testing.

@clue is planning to open a new ticket for this in PHPStan

You can touch it phpstan/phpstan#4245

In my opinion, with the introduction of Fibers, coroutines have become somewhat outdated

I agree. I'm planning to add supporting for Fibers in the future. But we will still not give up support for generators and promises.

SimonFrings Apr 17, 2024
Maintainer

I can see where you're coming from and I agree that better type hinting is always a good thing, but reactphp/promise#254 was not the way to go about this. We are always happy about contributions to our projects and in order to introduce new additions properly, all changes should be well tested and should have a reasonable explanation on why they are being added.

ReactPHP

PromiseInterface: add @yield annotation #536

Uh oh!

Uh oh!

roxblnfk Sep 13, 2023

Replies: 1 comment · 5 replies

Uh oh!

Uh oh!

yadaiio Oct 24, 2023

Uh oh!

roxblnfk Oct 24, 2023 Author

Uh oh!

WyriHaximus Oct 24, 2023 Maintainer

Uh oh!

SimonFrings Oct 27, 2023 Maintainer

Uh oh!

Uh oh!

roxblnfk Oct 27, 2023 Author

Uh oh!

SimonFrings Apr 17, 2024 Maintainer

PromiseInterface: add `@yield` annotation #536

roxblnfk
Sep 13, 2023

Replies: 1 comment 5 replies

yadaiio
Oct 24, 2023

roxblnfk Oct 24, 2023
Author

WyriHaximus Oct 24, 2023
Maintainer

SimonFrings Oct 27, 2023
Maintainer

roxblnfk Oct 27, 2023
Author

SimonFrings Apr 17, 2024
Maintainer