Skip to content

Tick callback receives no arguments, fix cyclic dependency #103

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

Merged
merged 2 commits into from
Apr 28, 2017

Conversation

clue
Copy link
Member

@clue clue commented Apr 26, 2017

The tick API has been documented as part of #100, however it makes no mention of how the tick function should look like. It actually currently receives the loop instance, which is entirely undocumented. The loop parameter also causes an unneeded cyclic dependency which hinders garbage collection.

This simple PR changes it so that the tick callback no longer receives a loop argument. This is obviously a small BC break, so this PR ensures to provide some upgrade guides to those affected by this. This is now also in line with the timer API, see #102.

Builds on top of #100 and refs #102

Remove loop argument and suggest using closure binding instead

// old (loop passed as parameter)
$loop->futureTick(function ($loop) {
    $loop->stop();
});

// already supported before: use closure binding as usual
$loop->futureTick(function () use ($loop) {
    $loop->stop();
});

@clue clue added this to the v0.5.0 milestone Apr 26, 2017
@clue clue removed the maintenance label Apr 26, 2017
* @see LoopInterface
* @internal
*/
final class FutureTickQueue
{
private $eventLoop;
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this anymore right?

@WyriHaximus WyriHaximus requested review from nrk and jsor April 26, 2017 14:31
@clue
Copy link
Member Author

clue commented Apr 26, 2017

@WyriHaximus Thanks for spotting, updated and squashed! :shipit:

@WyriHaximus
Copy link
Member

@clue LGTM :shipit:

@WyriHaximus WyriHaximus merged commit eaaed7c into reactphp:master Apr 28, 2017
@clue clue deleted the ticks branch April 28, 2017 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants