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

Remove BufferedSink #45

Merged
merged 1 commit into from Apr 27, 2017
Merged

Conversation

clue
Copy link
Member

@clue clue commented May 20, 2016

This ticket aims to serve as a discussion basis whether this class should actually be part of this repository.

Bridging between Stream-land and Promise-land is arguably out of scope for basic streams. Also, this happens to be the only class that has an "optional dependency" on promises. Full promise support would also involve proper cancellation support, stricter promise version checks and much more…

As an alternative, I would suggest making https://github.com/clue/php-promise-stream-react an official react repo and redirect there.

Builds on top of #44, also solves #24.

@WyriHaximus
Copy link
Member

I'm in favor of both suggestions, also I assume this is for 0.5 then?

@clue
Copy link
Member Author

clue commented May 20, 2016

I assume this is for 0.5 then

Yeah, this obviously involves a BC break and should target the v0.5 release.

@WyriHaximus
Copy link
Member

I assume this is for 0.5 then

Yeah, this obviously involves a BC break and should target the v0.5 release.

As long as https://github.com/clue/php-promise-stream-react becomes an official repo 👍 from me

@jsor
Copy link
Member

jsor commented May 25, 2016

I assume this is for 0.5 then

Yeah, this obviously involves a BC break and should target the v0.5 release.

As long as https://github.com/clue/php-promise-stream-react becomes an official repo 👍 from me

👍

@clue clue added this to the v0.5 milestone Aug 14, 2016
@clue
Copy link
Member Author

clue commented Aug 14, 2016

Rebased to remove merge conflicts now that #44 is in. This is currently planned for the future v0.5.0 release.

@clue clue changed the title [RFC] Remove BufferedSink Remove BufferedSink Apr 23, 2017
@clue
Copy link
Member Author

clue commented Apr 23, 2017

Rebased on current master :shipit:

I'd like to get this in for the next release so we can get the v1.0.0 release out and then later look into providing an "official" replacement. In the meantime, https://github.com/clue/php-promise-stream-react works just fine 👍

@WyriHaximus
Copy link
Member

In the meantime, https://github.com/clue/php-promise-stream-react works just fine 👍

How about we move that to an official repo and package? It looks good to me 👍 . Preferable before merging this/releasing this change.

@clue
Copy link
Member Author

clue commented Apr 23, 2017

In the meantime, https://github.com/clue/php-promise-stream-react works just fine 👍

How about we move that to an official repo and package?

I'm not opposed to this, but would rather focus our efforts on the main components first and then look into this? 👍

@WyriHaximus
Copy link
Member

WyriHaximus commented Apr 23, 2017

In the meantime, https://github.com/clue/php-promise-stream-react works just fine 👍

How about we move that to an official repo and package?

I'm not opposed to this, but would rather focus our efforts on the main components first and then look into this? 👍

Agreed, mainly wanted to point out that I think it is a bad plan to remove this without official replacement 😄

@clue
Copy link
Member Author

clue commented Apr 27, 2017

For the reference: The buffer() function is now part of react/promise-stream.

@WyriHaximus WyriHaximus merged commit a9add11 into reactphp:master Apr 27, 2017
@clue clue deleted the no-bufferedsink branch April 27, 2017 14:23
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.

None yet

3 participants