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

Include timeout logic to avoid dependency on reactphp/promise-timer #305

Merged
merged 1 commit into from
Jun 7, 2023

Conversation

clue
Copy link
Member

@clue clue commented Jun 6, 2023

This changeset adds timeout logic to avoid the otherwise unneeded dependency on reactphp/promise-timer. There are plans to deprecate the reactphp/promise-stream package as discussed in https://github.com/orgs/reactphp/discussions/475 and in either case I'd like to remove additional dependencies where possible.

In particular, reactphp/socket appears to be the only ReactPHP packages that still depends on reactphp/promise-timer at the moment. This means that once this PR is merged, most consumers of this package would no longer install reactphp/promise-timer by default. Perhaps most notably, this means that IDE autocompletion for resolve() and reject() would now only suggest the correct React\Promise\resolve() and React\Promise\reject() functions instead of the rarely useful and deprecated ones (see reactphp/promise-timer#51).

This changeset does not otherwise affect our public API, so this should be safe to apply. The test suite confirms this has 100% code coverage and does not otherwise affect our APIs.

Note that this changeset does not preclude the discussion in https://github.com/orgs/reactphp/discussions/475, so whether or not or when the package will be deprecated is still up for debate. If you want to explicitly install this dependency, you can still install it like this:

composer require react/promise-timer

Builds on top of reactphp/dns#213 and others
See also reactphp/http#482 for similar changes to remove reactphp/promise-stream from reactphp/http

@clue
Copy link
Member Author

clue commented Jun 6, 2023

Note the build error on legacy PHP 5.4 on Windows is unrelated to this changeset, see #306 for workaround. We can either proceed with applying this PR with a known build error or merge the other PR first and rebase this one, either works for me 👍

@WyriHaximus
Copy link
Member

Note the build error on legacy PHP 5.4 on Windows is unrelated to this changeset, see #306 for workaround. We can either proceed with applying this PR with a known build error or merge the other PR first and rebase this one, either works for me +1

That error just blows my mind. But approved and merged it.

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.

3 participants