Make async return a callable
#19
Conversation
8105817 to
7b12a9d
Compare
src/functions.php
Outdated
| * @param callable(mixed ...$args):mixed $function | ||
| * @param mixed ...$args Optional list of additional arguments that will be passed to the given `$function` as is | ||
| * @return PromiseInterface<mixed> | ||
| * @return callable(): PromiseInterface<mixed> |
There was a problem hiding this comment.
The $args parameter should be omitted here and below.
How about this?
* @param callable(mixed ...$args): mixed $function
* @return callable(mixed ...$args): PromiseInterface<mixed>
There was a problem hiding this comment.
This is actually unsafe from a static analysis perspective, it would be better to get rid of it completely.
There was a problem hiding this comment.
@azjezz Why is it unsafe from a static analysis perspective?
There was a problem hiding this comment.
$callable = function(int $a): void {}
$async_callable = async($callable, new User()); // <-- both psalm, and phpstan allow this, but it's wrong.
There was a problem hiding this comment.
and passing outside arguments is not hard in PHP 8.1, async(fn() => foo($a, $b, $c))
There was a problem hiding this comment.
@azjezz I'm not sure I follow. With these changes, the async() function no longer accepts any arguments, but it returns a new function that accepts any number of arguments that will be forwarded as-is to the original $function. Are you aware of a better type definition for this?
There was a problem hiding this comment.
no, there's no way to actually type this in PHP ( and most other typed languages ).
async is currently: (F(...unknown) -> Y) + ...unknown -> (F() -> Promise)
your suggestion: (F(...unknown) -> Y) -> (F(...unknown) -> Promise)
The issue is not just that we don't know type of 'unknown', but also how many.
so even this:
$async = async(fn(string $identifier): User => $db->getUser($identifier));
$result = await($async([1, 2, 3]));will pass static analysis since the returned callable is annotated as callable(mixed ...$args): Promise<mixed>, but fails at runtime.
( aside from dropping $args, it would be nice to add generics so that the return type of callable is known, as of now, $result in the example above is detected as mixed while it should be User )
ref: https://github.com/azjezz/psl/blob/2.0.x/src/Psl/Async/run.php#L15-L19
There was a problem hiding this comment.
@azjezz I think we're on the same page here. I think we agree that the @param mixed ...$params should be removed and @return callable(): PromiseInterface<mixed> is probably good enough as a return type until we see better support for this in static analysis tools 👍
7b12a9d to
37420b0
Compare
|
This works for simple cases but I'm not sure if the DX proposed in this PR of wrapping your callback with In the current situation, when no
So currently, this still fails: $thirdPartyLibrary = function () {
await(async(fn() => print 'tick' . PHP_EOL)());
};
Loop::addTimer(5, $thirdPartyLibrary);This is what's proposed in this PR, but probably isn't desirable and also doesn't work: $thirdPartyLibrary = function () {
await(async(fn() => print 'tick' . PHP_EOL)());
};
Loop::addTimer(5, async($thirdPartyLibrary)());I've talked to @WyriHaximus every now and then recently and he suggested to remove the timer before the fiber is suspended. Is there a reason that's not an option? Also, the problem with wrapping async calls might still be a problem in that situation and it might be unrelated to this issue/PR. Please do tell me if I'm being an idiot 😃. |
|
@bartvanhoutte I'm not sure I follow exactly what problem you're seeing, but I agree that we still have to work out the best DX nonetheless! Here's one of your examples that should work with the above suggestion: $thirdPartyLibrary = function () {
await(async(fn() => print 'tick' . PHP_EOL)());
};
Loop::addTimer(5, async($thirdPartyLibrary));I agree that it's easy to mix up the parenthesis and this is definitely something we may want to look in to. Here's a more realistic use case showing the exact same syntax, but hopefully less confusing: Loop::addTimer(5, async(function () {
$browser = React\Http\Browser();
$response = await($browser->get('https://example.com/'));
assert($response instanceof Psr\Http\Message\Response);
var_dump($response);
}));The gist is that any You're looking at an early development version at the moment and I completely agree that we still have to work on the DX and documentation at the very least! |
|
@clue That first example does the trick! Mixed up some parantheses indeed. Since that works, is there a reason to not wrap the callable with
I figured, perfect. Super to work with so far! |
Creating a fiber, promise and waiting for a future tick incurs some noticeable overhead, so our current position is to only use this when actually required. There's no way for us to analyze the callable, so it's currently up to the developer to manually invoke the |
|
My hopes for the future are, but for this we have to wrap it in functions: Loop::addTimer(5, asyncfunction () {
$browser = React\Http\Browser();
$response = await $browser->get('https://example.com/');
assert($response instanceof Psr\Http\Message\Response);
var_dump($response);
}); |
| { | ||
| return new Promise(function (callable $resolve, callable $reject) use ($function, $args): void { | ||
| return static fn (mixed ...$args): PromiseInterface => new Promise(function (callable $resolve, callable $reject) use ($function, $args): void { |
There was a problem hiding this comment.
| return static fn (mixed ...$args): PromiseInterface => new Promise(function (callable $resolve, callable $reject) use ($function, $args): void { | |
| return static fn (): PromiseInterface => new Promise(function (callable $resolve, callable $reject) use ($function): void { |
as discussed here: https://github.com/reactphp/async/pull/19/files#r773363277
There was a problem hiding this comment.
@azjezz Only dropped the docblock item as this suggested change will break it
There was a problem hiding this comment.
@azjezz The changes LGTM as is, there's definitely value in being able to pass additional arguments through to the original function.
By making this change `async()` can now also by used in event loop callbacks such as timers:
```php
Loop::addTimer(0.01, async(function () {
echo 'Sleeping for one second';
await(asleep(1));
echo 'Waking up again';
}));
```
With this change, current `async()` usage changes from:
```php
async(function () {
//
});
```
To:
```php
async(function () {
//
})();
```
37420b0 to
4355fcf
Compare
Since `async()` returns a promise and those are normally cancelable, implementing this puts them in line with the rest of our ecosystem. As such the following example will throw a timeout exception from the canceled `sleep()` call.
```php
$promise = async(static function (): int {
echo 'a';
await(sleep(2));
echo 'b';
return time();
})();
$promise->cancel();
await($promise);
````
This builds on top of reactphp#15, reactphp#18, reactphp#19, reactphp#26, reactphp#28, reactphp#30, and reactphp#32.
Since `async()` returns a promise and those are normally cancelable, implementing this puts them in line with the rest of our ecosystem. As such the following example will throw a timeout exception from the canceled `sleep()` call.
```php
$promise = async(static function (): int {
echo 'a';
await(sleep(2));
echo 'b';
return time();
})();
$promise->cancel();
await($promise);
````
This builds on top of reactphp#15, reactphp#18, reactphp#19, reactphp#26, reactphp#28, reactphp#30, and reactphp#32.
Since `async()` returns a promise and those are normally cancelable, implementing this puts them in line with the rest of our ecosystem. As such the following example will throw a timeout exception from the canceled `sleep()` call.
```php
$promise = async(static function (): int {
echo 'a';
await(sleep(2));
echo 'b';
return time();
})();
$promise->cancel();
await($promise);
````
This builds on top of reactphp#15, reactphp#18, reactphp#19, reactphp#26, reactphp#28, reactphp#30, and reactphp#32.
Since `as
9E88
ync()` returns a promise and those are normally cancelable, implementing this puts them in line with the rest of our ecosystem. As such the following example will throw a timeout exception from the canceled `sleep()` call.
```php
$promise = async(static function (): int {
echo 'a';
await(sleep(2));
echo 'b';
return time();
})();
$promise->cancel();
await($promise);
````
This builds on top of reactphp#15, reactphp#18, reactphp#19, reactphp#26, reactphp#28, reactphp#30, and reactphp#32.
Since `async()` returns a promise and those are normally cancelable, implementing this puts them in line with the rest of our ecosystem. As such the following example will throw a timeout exception from the canceled `sleep()` call.
```php
$promise = async(static function (): int {
echo 'a';
await(sleep(2));
echo 'b';
return time();
})();
$promise->cancel();
await($promise);
````
This builds on top of reactphp#15, reactphp#18, reactphp#19, reactphp#26, reactphp#28, reactphp#30, and reactphp#32.
Since `async()` returns a promise and those are normally cancelable, implementing this puts them in line with the rest of our ecosystem. As such the following example will throw a timeout exception from the canceled `sleep()` call.
```php
$promise = async(static function (): int {
echo 'a';
await(sleep(2));
echo 'b';
return time();
})();
$promise->cancel();
await($promise);
````
This builds on top of reactphp#15, reactphp#18, reactphp#19, reactphp#26, reactphp#28, reactphp#30, and reactphp#32.
Since `async()` returns a promise and those are normally cancelable, implementing this puts them in line with the rest of our ecosystem. As such the following example will throw a timeout exception from the canceled `sleep()` call.
```php
$promise = async(static function (): int {
echo 'a';
await(sleep(2));
echo 'b';
return time();
})();
$promise->cancel();
await($promise);
````
This builds on top of reactphp#15, reactphp#18, reactphp#19, reactphp#26, reactphp#28, reactphp#30, and reactphp#32.
Since `async()` returns a promise and those are normally cancelable, implementing this puts them in line with the rest of our ecosystem. As such the following example will throw a timeout exception from the canceled `sleep()` call.
```php
$promise = async(static function (): int {
echo 'a';
await(sleep(2));
echo 'b';
return time();
})();
$promise->cancel();
await($promise);
````
This builds on top of reactphp#15, reactphp#18, reactphp#19, reactphp#26, reactphp#28, reactphp#30, and reactphp#32.
Since `async()` returns a promise and those are normally cancelable, implementing this puts them in line with the rest of our ecosystem. As such the following example will throw a timeout exception from the canceled `sleep()` call.
```php
$promise = async(static function (): int {
echo 'a';
await(sleep(2));
echo 'b';
return time();
})();
$promise->cancel();
await($promise);
````
This builds on top of reactphp#15, reactphp#18, reactphp#19, reactphp#26, reactphp#28, reactphp#30, and reactphp#32.
Since `async()` returns a promise and those are normally cancelable, implementing this puts them in line with the rest of our ecosystem. As such the following example will throw a timeout exception from the canceled `sleep()` call.
```php
$promise = async(static function (): int {
echo 'a';
await(sleep(2));
echo 'b';
return time();
})();
$promise->cancel();
await($promise);
````
This builds on top of reactphp#15, reactphp#18, reactphp#19, reactphp#26, reactphp#28, reactphp#30, and reactphp#32.
By making this change
async()can now also by used in event loop callbacks such as timers:With this change, current
async()usage changes from:To: