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

Deal with iterable in implementations of PromiseAdapter::all() #1263

Merged
merged 2 commits into from
Dec 19, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ You can find and compare releases at the [GitHub release page](https://github.co
- Avoid calling `FormattedError::addDebugEntries()` twice when using default error formatting
- Avoid calling defined functions named like lazily loaded types
- Show actual error in debug entries
- Deal with `iterable` in implementations of `PromiseAdapter::all()`

### Removed

Expand Down
18 changes: 8 additions & 10 deletions src/Executor/Promise/Adapter/AmpPromiseAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,6 @@ public function createRejected(\Throwable $reason): Promise

public function all(iterable $promisesOrValues): Promise
{
\assert(
\is_array($promisesOrValues),
'AmpPromiseAdapter::all(): Argument #1 ($promisesOrValues) must be of type array'
);

/** @var array<AmpPromise<mixed>> $promises */
$promises = [];
foreach ($promisesOrValues as $key => $item) {
Expand All @@ -98,18 +93,21 @@ public function all(iterable $promisesOrValues): Promise

$deferred = new Deferred();

$onResolve = static function (?\Throwable $reason, ?array $values) use ($promisesOrValues, $deferred): void {
all($promises)->onResolve(static function (?\Throwable $reason, ?array $values) use ($promisesOrValues, $deferred): void {
if ($reason === null) {
\assert(\is_array($values), 'Either $reason or $values must be passed');
$deferred->resolve(\array_replace($promisesOrValues, $values));

$promisesOrValuesArray = is_array($promisesOrValues)
? $promisesOrValues
: iterator_to_array($promisesOrValues);
$resolvedValues = \array_replace($promisesOrValuesArray, $values);
$deferred->resolve($resolvedValues);

return;
}

$deferred->fail($reason);
};

all($promises)->onResolve($onResolve);
});

return new Promise($deferred->promise(), $this);
}
Expand Down
12 changes: 5 additions & 7 deletions src/Executor/Promise/Adapter/ReactPromiseAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -56,20 +56,18 @@ public function createRejected(\Throwable $reason): Promise

public function all(iterable $promisesOrValues): Promise
{
assert(
is_array($promisesOrValues),
'ReactPromiseAdapter::all(): Argument #1 ($promisesOrValues) must be of type array'
);

foreach ($promisesOrValues as &$promiseOrValue) {
if ($promiseOrValue instanceof Promise) {
$promiseOrValue = $promiseOrValue->adoptedPromise;
}
}

$promise = all($promisesOrValues)->then(static fn ($values): array => array_map(
$promisesOrValuesArray = is_array($promisesOrValues)
? $promisesOrValues
: iterator_to_array($promisesOrValues);
$promise = all($promisesOrValuesArray)->then(static fn ($values): array => array_map(
static fn ($key) => $values[$key],
array_keys($promisesOrValues),
array_keys($promisesOrValuesArray),
));

return new Promise($promise, $this);
Expand Down
7 changes: 3 additions & 4 deletions src/Executor/Promise/Adapter/SyncPromise.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,10 @@
* Simplistic (yet full-featured) implementation of Promises A+ spec for regular PHP `sync` mode
* (using queue to defer promises execution).
*
* Note:
* Library users are not supposed to use SyncPromise class in their resolvers.
* Instead they should use GraphQL\Deferred which enforces $executor callback in the constructor.
* Instead, they should use @see \GraphQL\Deferred which enforces `$executor` callback in the constructor.
*
* Root SyncPromise without explicit $executor will never resolve (actually throw while trying).
* Root SyncPromise without explicit `$executor` will never resolve (actually throw while trying).
* The whole point of Deferred is to ensure it never happens and that any resolver creates
* at least one $executor to start the promise chain.
*
Expand Down Expand Up @@ -42,7 +41,7 @@ class SyncPromise
* }
* >
*/
private array $waiting = [];
protected array $waiting = [];

public static function runQueue(): void
{
Expand Down
27 changes: 12 additions & 15 deletions src/Executor/Promise/Adapter/SyncPromiseAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -71,29 +71,28 @@ public function createRejected(\Throwable $reason): Promise

public function all(iterable $promisesOrValues): Promise
{
\assert(
\is_array($promisesOrValues),
'SyncPromiseAdapter::all(): Argument #1 ($promisesOrValues) must be of type array'
);

$all = new SyncPromise();

$total = \count($promisesOrValues);
$total = \is_array($promisesOrValues)
? \count($promisesOrValues)
: \iterator_count($promisesOrValues);
$count = 0;
$result = [];

$resolveAllWhenFinished = function () use (&$count, &$total, $all, &$result): void {
if ($count === $total) {
$all->resolve($result);
}
};

foreach ($promisesOrValues as $index => $promiseOrValue) {
if ($promiseOrValue instanceof Promise) {
$result[$index] = null;
$promiseOrValue->then(
static function ($value) use ($index, &$count, $total, &$result, $all): void {
static function ($value) use (&$result, $index, &$count, &$resolveAllWhenFinished): void {
$result[$index] = $value;
++$count;
if ($count < $total) {
return;
}

$all->resolve($result);
$resolveAllWhenFinished();
},
[$all, 'reject']
);
Expand All @@ -103,9 +102,7 @@ static function ($value) use ($index, &$count, $total, &$result, $all): void {
}
}

if ($count === $total) {
$all->resolve($result);
}
$resolveAllWhenFinished();

return new Promise($all, $this);
}
Expand Down
2 changes: 1 addition & 1 deletion src/Server/Helper.php
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,8 @@ public function executeOperation(ServerConfig $config, OperationParams $op)
public function executeBatch(ServerConfig $config, array $operations)
{
$promiseAdapter = $config->getPromiseAdapter() ?? Executor::getPromiseAdapter();
$result = [];

$result = [];
foreach ($operations as $operation) {
$result[] = $this->promiseToExecuteOperation($promiseAdapter, $config, $operation, true);
}
Expand Down
17 changes: 5 additions & 12 deletions tests/Executor/Promise/ReactPromiseAdapterTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,16 @@
/**
* @group ReactPromise
*/
class ReactPromiseAdapterTest extends TestCase
final class ReactPromiseAdapterTest extends TestCase
{
public function testIsThenableReturnsTrueWhenAReactPromiseIsGiven(): void
{
$reactAdapter = new ReactPromiseAdapter();

self::assertTrue(
$reactAdapter->isThenable(new ReactPromise(static function (): void {
}))
);
self::assertTrue($reactAdapter->isThenable(new ReactPromise(static fn () => null)));
self::assertTrue($reactAdapter->isThenable(resolve()));
self::assertTrue($reactAdapter->isThenable(reject()));
self::assertFalse($reactAdapter->isThenable(static fn () => null));
self::assertFalse($reactAdapter->isThenable(false));
self::assertFalse($reactAdapter->isThenable(true));
self::assertFalse($reactAdapter->isThenable(1));
Expand Down Expand Up @@ -57,7 +55,6 @@ public function testThen(): void
$promise = $reactAdapter->convertThenable($reactPromise);

$result = null;

$resultPromise = $reactAdapter->then(
$promise,
static function ($value) use (&$result): void {
Expand All @@ -79,7 +76,6 @@ public function testCreate(): void
self::assertInstanceOf(Promise::class, $resolvedPromise->adoptedPromise);

$result = null;

$resolvedPromise->then(static function ($value) use (&$result): void {
$result = $value;
});
Expand All @@ -95,7 +91,6 @@ public function testCreateFulfilled(): void
self::assertInstanceOf(FulfilledPromise::class, $fulfilledPromise->adoptedPromise);

$result = null;

$fulfilledPromise->then(static function ($value) use (&$result): void {
$result = $value;
});
Expand All @@ -111,15 +106,14 @@ public function testCreateRejected(): void
self::assertInstanceOf(RejectedPromise::class, $rejectedPromise->adoptedPromise);

$exception = null;

$rejectedPromise->then(
null,
static function ($error) use (&$exception): void {
$exception = $error;
}
);

self::assertInstanceOf('\Exception', $exception);
self::assertInstanceOf(\Exception::class, $exception);
self::assertEquals('I am a bad promise', $exception->getMessage());
}

Expand All @@ -133,7 +127,6 @@ public function testAll(): void
self::assertInstanceOf(FulfilledPromise::class, $allPromise->adoptedPromise);

$result = null;

$allPromise->then(static function ($values) use (&$result): void {
$result = $values;
});
Expand All @@ -146,8 +139,8 @@ public function testAllShouldPreserveTheOrderOfTheArrayWhenResolvingAsyncPromise
$reactAdapter = new ReactPromiseAdapter();
$deferred = new Deferred();
$promises = [resolve(1), $deferred->promise(), resolve(3)];
$result = null;

$result = null;
$reactAdapter->all($promises)->then(static function ($values) use (&$result): void {
$result = $values;
});
Expand Down