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.

Sign up for GitHub

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

Jump to bottom

Support Connector without DNS #46

Merged
merged 5 commits into from
Nov 21, 2015
Merged

Support Connector without DNS #46

merged 5 commits into from
Nov 21, 2015

Conversation

clue
Copy link
Contributor

@clue clue commented Sep 7, 2015

  • Split Connector into TcpConnector and DnsConnector
  • Legacy Connector now acts as a proxy for BC only

This 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

@clue
Copy link
Contributor Author

clue commented Sep 7, 2015

I consider this feature complete, though this is probably blocked until #43 is merged. As such, I've marked this WIP for now.

Any feedback is welcome! Perhaps this PR also helps to emphasize my motivation for the API used in #43/#45.

This was referenced Sep 7, 2015
Closed
Closed
Closed
@clue clue added this to the v0.5 milestone Sep 22, 2015
@clue clue added the BC break label Sep 22, 2015
@clue
Copy link
Contributor Author

clue commented Sep 23, 2015

I've just updated this PR so that it no longer includes a BC break.

@clue clue added new feature and removed BC break labels Sep 23, 2015
@clue clue modified the milestones: v0.4.5, v0.5 Sep 23, 2015
@clue clue changed the title [WIP] Support Connector without DNS Support Connector without DNS Sep 24, 2015
@clue
Copy link
Contributor Author

clue commented Sep 24, 2015

I've just rebased this on the current master now that #43 is in.

Ready for review :) Ping @WyriHaximus, @cboden

@WyriHaximus
Copy link
Contributor

WyriHaximus commented Sep 24, 2015 via email

@clue
Copy link
Contributor Author

clue commented Oct 13, 2015

I've just updated this PR so that it no longer includes a BC break.

This currently does not break compatibility what's been documented in our README, however the public interface of the Connector class had to be changed. Should we consider this a BC break, i.e. should this be v0.4.5 or v0.5.0?

This was referenced Oct 13, 2015
Merged
Merged
@WyriHaximus
Copy link
Contributor

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

@clue
Copy link
Contributor Author

clue commented Nov 18, 2015

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:

For the reference, not a single one of the examples instantiating the Connector and calling its create() method is affected.

Only the two classes that extend the Connector and access its protected members would be affected.

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.

:shipit: ? :)

@clue clue modified the milestones: v0.5, v0.4.5 Nov 18, 2015
@clue clue added the BC break label Nov 18, 2015
@clue clue mentioned this pull request Nov 20, 2015
Merged
jsor
jsor reviewed Nov 20, 2015
View reviewed changes
src/TcpConnector.php

if (!$socket) {
return Promise\reject(new \RuntimeException(
sprintf("connection to %s:%d failed: %s", $ip, $port, $errstr),
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: connection -> Connection

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for spotting, fixed! :)

For the reference: This code has been like this ever since the very first commit in this repo.

@jsor
Copy link
Contributor

jsor commented Nov 20, 2015

A small typo, otherwise LGTM :shipit:

@WyriHaximus
Copy link
Contributor

LGTM :shipit:

clue added a commit that referenced this pull request Nov 21, 2015
@clue
Merge pull request #46 from clue-labs/resolving
0460210 
Support Connector without DNS
@clue clue merged commit 0460210 into reactphp-legacy:master Nov 21, 2015
@clue clue mentioned this pull request Nov 21, 2015
Closed
@clue clue mentioned this pull request Mar 4, 2016
Closed
@clue clue mentioned this pull request Feb 8, 2017
Closed
@clue clue deleted the resolving branch February 12, 2017 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
BC break new feature
Projects
None yet
Milestone
v0.5.0
Development

Successfully merging this pull request may close these issues.

Allow Connector to have no Resolver
3 participants
@clue @WyriHaximus @jsor