8000 Documentation for timer API and clean up unneeded timer API by clue · Pull Request #102 · reactphp/event-loop · GitHub
[go: up one dir, main page]
Skip to content

Documentation for timer API and clean up unneeded timer API #102

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Apr 28, 2017
Merged

Documentation for timer API and clean up unneeded timer API #102

merged 5 commits into from
Apr 28, 2017

Conversation

clue
Copy link
Member
@clue clue commented Apr 26, 2017

The timer API is currently entirely undocumented and offers several duplicate methods and accessors not really required. Arguably, this makes the timer API appear more difficult than it actually is. The loop accessors also causes an unneeded cyclic dependency which hinders garbage collection.

This PR adds documentation for the existing timer API and then subsequently removes unneeded methods and the cyclic dependency. Note that while this PR does remove parts of the existing API, it does not add any new APIs or otherwise modify the existing APIs. While this does contain some BC breaks, many consumers will not be affected by this due to the duplicate methods. This PR ensures to provide some upgrade guides to those affected by the BC breaks.

Performance improvement is not a major motivation here, but shows a negligible 3-5% improvement anyway (running examples 92 and 94).

If you want to review, consider also looking at the individual commits.

Builds on top of #99 and #100.

cancelTimer() and isTimerActive()

// old (methods invoked on timer instance)
$timer->cancel();
assert(!$timer->isActive());

// already supported before: invoke methods on loop instance
$loop->cancelTimer($timer);
assert(!$loop->isTimerActive($timer));

Use closure binding to add arbitrary data to timer

// old (limited setData() and getData() only allows single variable)
$name = 'Tester';
$timer = $loop->addTimer(1.0, function ($timer) {
    echo 'Hello ' . $timer->getData() . PHP_EOL;
});
$timer->setData($name);

// already supported before: closure binding allows any number of variables
$name = 'Tester';
$loop->addTimer(1.0, function () use ($name) {
    echo 'Hello ' . $name . PHP_EOL;
});

Remove getLoop() and suggest using closure binding instead

// old (getLoop() called on timer instance)
$loop->addTimer(0.1, function ($timer) {
    $timer->getLoop()->addTimer(0.1, function () {
        echo 'Hello';
    });
});

// already supported before: use closure binding as usual
$loop->addTimer(0.1, function () use ($loop) {
    $loop->addTimer(0.1, function () {
        echo 'Hello';
    });
});

@clue clue added this to the v0.5.0 milestone Apr 26, 2017
clue added 5 commits April 26, 2017 11:34
@clue
Documentation for timer API
b0e7215
@clue
Remove unneeded and undocumented timer data, use closure binding instead
7eb31c1 
The mixed "data" attribute is mostly unused and can easily be replaced
by closure binding instead.

This also prepares the timer instance to become an immutable
data structure.
@clue
Remove unneeded duplicate methods cancel() and isActive()
2f8b16a 
These can easily be replaced by Loop::cancelTimer() and
Loop::isTimerActive().

This change ensures that timers are now a plain immutable data structure
and offer no methods to change its state.

This also prepares the timer to remove the cyclic dependency with the
loop instance.
@clue
Remove unneeded getLoop() method and cyclic dependency with loop
cc8995f 
Thid can easily be replaced by using closure binding as documented.
@clue
Mark internal timer API as final to discourage external usage
a6acc48
@clue clue mentioned this pull request Apr 26, 2017
Merged
@clue clue removed the maintenance label Apr 26, 2017
@WyriHaximus WyriHaximus requested review from nrk, jsor and WyriHaximus April 26, 2017 14:31
nrk
nrk approved these changes Apr 27, 2017
View reviewed changes
Copy link
Member
@nrk nrk left a comment

Choose a reason for hiding this comment

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

These changes definitely make sense at this stage of ReactPHP. The current API defined by React\EventLoop\TimerInterface was motivated by an hypothetical transition to a full object-based approach that wrapped resources managed by the event loop, but this vision never really took shape for various reasons (even practical ones). Probably this was never discussed in the public or maybe just on IRC, this is something that happened at the very early stages of the project after all.

@WyriHaximus WyriHaximus merged commit 9483983 into reactphp:master Apr 28, 2017
@clue clue deleted the timers branch April 28, 2017 19:39
@clue clue mentioned this pull request Oct 13, 2017
Merged
@clue clue mentioned this pull request Nov 10, 2017
Merged
@clue clue mentioned this pull request Dec 4, 2017
Merged
@clue clue mentioned this pull request Dec 23, 2017
Merged
@clue clue mentioned this pull request Apr 3, 2018
Merged
This was referenced Jul 29, 2018
Merged
Merged
@clue clue mentioned this pull request Jan 3, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nrk nrk nrk approved these changes

@jsor jsor jsor approved these changes

@WyriHaximus WyriHaximus WyriHaximus approved these changes

Assignees
No one assigned
Labels
BC break maintenance new feature
Projects
None yet
Milestone
v0.5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@clue @nrk @jsor @WyriHaximus
0