Support Connector without DNS#46
Conversation
|
I've just updated this PR so that it no longer includes a BC break. |
|
I've just rebased this on the current master now that #43 is in. Ready for review :) Ping @WyriHaximus, @cboden |
|
LGTM, this is a very welcome feature 👍
|
This currently does not break compatibility what's been documented in our README, however the public interface of the |
|
We might want to consider this a BC break as the publicly exposed API has changed. Also there are a ton of packages uses the class: http://packanalyst.com/class?q=React%5CSocketClient%5CConnector |
For the reference, not a single one of the examples instantiating the Only the two classes that I agree that we might want to play safe here and consider this a BC break nonetheless. The CHANGELOG should include some details that this doesn't affect most users though. Also, I've just marked the internal APIs as such, so this doesn't happen again if we change the internal APIs.
|
src/TcpConnector.php
Outdated
There was a problem hiding this comment.
Typo: connection -> Connection
There was a problem hiding this comment.
Thanks for spotting, fixed! :)
For the reference: This code has been like this ever since the very first commit in this repo.
|
A small typo, otherwise LGTM |
|
LGTM |
ConnectorintoTcpConnectorandDnsConnectorConnectornow acts as a proxy for BC onlyThis PR builds on top of #43, so the diff also includes its changes. Look here for changes only related to this PR alone.(no longer applies after rebasing)Fixes #12
Supersedes/closes #7