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

Simplify usage by supporting new default loop #16

Closed
wants to merge 1 commit into from

Conversation

PaulRotmann
Copy link
Contributor

This changeset simplifies usage by supporting the new default loop.
Builds on top of reactphp/event-loop#226, reactphp/event-loop#229, reactphp/event-loop#231, reactphp/event-loop#232.

Copy link
Owner

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PaulRotmann Thanks for looking into this, added some remarks below 👍

$resolver = $factory->createResolver();

$promise = $resolver->lookup('me.local');

try {
$ip = Block\await($promise, $loop);
$ip = Block\await($promise);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not supported by underlying await() function


```php
$loop = React\EventLoop\Factory::create();
$factory = new Factory($loop);
$factory = new Factory();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but could use some additional docs?

@@ -128,7 +123,7 @@ $promises = array(
$resolver->lookup('second.local'),
);

$ips = Block\awaitAll($promises, $loop);
$ips = Block\awaitAll($promises);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not supported, see above.

$loop = React\EventLoop\Factory::create();
$factory = new Clue\React\Mdns\Factory($loop);
$factory = new Clue\React\Mdns\Factory();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but example still references a leftover $loop?

@clue clue added this to the v0.2.1 milestone Aug 12, 2021
@clue clue closed this in #17 Sep 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants