Skip to content

Remove unneeded isTimerActive() to reduce API surface #133

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 1 commit into from
Dec 4, 2017

Conversation

clue
Copy link
Member

@clue clue commented Dec 4, 2017

Builds on top of #102

@WyriHaximus WyriHaximus merged commit f6dbe84 into reactphp:master Dec 4, 2017
@clue clue deleted the active branch December 4, 2017 15:39
@dearkey
Copy link

dearkey commented Jan 26, 2019

how to check now if a timer is active when isTimerActive() is removed?

@clue
Copy link
Member Author

clue commented Jan 27, 2019

@dearkey Good question, can you gist your use case and the problem you're trying to solve? During our discussions for this ticket and #102 we didn't come up with any use cases for this API, so we decided to remove it as part of the API cleanup. Likewise, this ticket hasn't been referenced for more than a year.

@dearkey
Copy link

dearkey commented Jan 27, 2019

i use now a global variable to set the active state. when i use a addPeriodicTimer to start an other one i have to check if its already running. because the second will be stopped when nothing is to do.

@clue
Copy link
Member Author

clue commented Jan 27, 2019

@dearkey The way I understand this, this sounds like a reasonable approach 👍

Using an additional variable to hold this application state is a pattern we see in a number of applications (e.g. https://github.com/clue/reactphp-ssh-proxy/blob/c1732bad827672d644ea28f4806e08b86271608a/src/SshSocksConnector.php#L168-L171 and many others) and it's my understanding this solves this problem much better than exposing this state in this lower-level component 👍

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.

4 participants