Skip to content

Conversation

@clue
Copy link
Contributor

@clue clue commented Nov 22, 2015

Because why not… :-)

Now more seriously: This project is a low level lib that is used as a building block for quite a few higher level abstractions on top of it. As such, compatibility (even with significantly outdated versions) is a major concern to me.

Note that I'm not suggesting putting significant amount of work into this. The patch is already here and, personally, I see little harm in supporting this.

Also note that I'm not suggesting we need to keep support indefinitely. Should this ever turn out to be a burden in the future, e.g. because we actually require any new language features or some external lib, then I'm all for dropping support again.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about code that typehints against DuplexStreamInterface? Considering to be a BC tbh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This actually amends #26 which in turn amends #14.

The documentation always stated this returns a Stream instance, never an instance implementing DuplexStreamInterface.

FWIW: This repo currently targets react/stream:0.4.*, while this interface has only been added with v0.4.2, so even if we decide not to support legacy versions, this is still a bug that needs fixing. If you happen to have v0.4.2+, then Stream implements the DuplexStreamInterface.

As such, I do not consider this a BC break.

Even despite this, this PR should probably be part of the v0.5.0 milestone, which already includes a (minor) BC break anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wasn't aware DuplexStreamInterface was added later in 0.4.x, so yeah it isn't a BC break imo :).

@clue clue added this to the v0.5 milestone Nov 22, 2015
@cboden
Copy link
Contributor

cboden commented Mar 8, 2016

Not quite sure what it was but I get a failing unit test when merging this into master (even after resolving the merge conflict).

@clue
Copy link
Contributor Author

clue commented Mar 8, 2016

Not quite sure what it was but I get a failing unit test when merging this into master (even after resolving the merge conflict).

I've just rebased this now that #52 is in and all tests are working just fine 👍 What error are you seeing?

@cboden cboden merged commit 1279cb3 into reactphp-legacy:master Mar 8, 2016
@cboden
Copy link
Contributor

cboden commented Mar 8, 2016

Rebase looks good. I must have messed up the merge on my end before

@clue clue deleted the compat branch December 5, 2016 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants