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 buffer logic to avoid dependency on reactphp/promise-stream #482

Merged
merged 1 commit into from
Nov 29, 2022

Conversation

clue
Copy link
Member

@clue clue commented Nov 29, 2022

This changeset adds buffering logic to avoid the otherwise unneeded dependency on reactphp/promise-stream. There are plans to deprecate that 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/http appears to be the only ReactPHP package that depends on reactphp/promise-stream at the moment. This means that most consumers of this package would no longer install reactphp/promise-stream at all. Perhaps most notably, this means that IDE autocompletion for all() would now only suggest the correct React\Promise\all() function instead of the rarely useful React\Promise\Stream\all() function.

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. This improves error reporting slightly, but I've tried to keep changes to a minimum otherwise.

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-stream

Builds on top of #480, #460 and others
Refs #468

@clue clue added this to the v1.9.0 milestone Nov 29, 2022
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