Skip to content

"Fix" minor BC break #25

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 2 commits into from
Mar 20, 2015
Merged

"Fix" minor BC break #25

merged 2 commits into from
Mar 20, 2015

Conversation

cboden
Copy link
Contributor

@cboden cboden commented Feb 3, 2015

Whelp...this fixes the issue...SecureStream extends Stream for the API compatibility but is still completely acting like a decorator. It feels hackey but it works with minimal changes and allows the simplest migration path to 0.5.

@cboden
Copy link
Contributor Author

cboden commented Feb 3, 2015

refs #21

@cboden cboden added this to the v0.4.3 milestone Feb 3, 2015
@clue
Copy link
Contributor

clue commented Feb 5, 2015

👍 on updating the SecureStream

The SecureStream should extend the Stream class in order to avoid the BC break introduced via #14 in v0.4.2. This fixes #21.

An alternative would be to update the documentation and state that we only return an object that implements the DuplexStreamInterface and release a v0.5.0 tag. In that case we would have to depend on react/stream: >= 0.4.2, < 0.5.0.

👎 on updating the README.md

As discussed in #21 this would introduce a (subtle) BC break. This PR is linked to the "v0.4.3" milestone. I'm okay with doing so in a separate PR that targets a "v0.5.0" milestone though.

@clue
Copy link
Contributor

clue commented Feb 5, 2015

BTW, this could use a test: Both Connector and SecureConnector should resolve with an instance of Stream.

@clue clue mentioned this pull request Feb 19, 2015
@clue
Copy link
Contributor

clue commented Mar 11, 2015

LGTM 👍

Any chance to get a test included? (See #16 or #18).

@cboden cboden merged commit 14116e4 into master Mar 20, 2015
@cboden cboden deleted the bc-fix branch March 20, 2015 16:02
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.

2 participants