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

Connections now resolve with a ConnectionInterface #82

Merged
merged 1 commit into from
Feb 16, 2017

Conversation

clue
Copy link
Contributor

@clue clue commented Feb 16, 2017

This PR changes the resolution value from Stream (which implements the DuplexStreamInterface) to an instance implementing the new ConnectionInterface, which in turn implements the DuplexStreamInterface.

In other words: Each ConnectorInterface now has a connect() method that resolves to a ConnectionInterface.

Because the ConnectionInterface implements the same base interface DuplexStreamInterface, it behaves just like a normal stream resource, so consumer code can simply use the new typehint and continue to work unchanged.

The ConnectionInterface also implements two new methods which are not available for the underlying stream:

  • getRemoteAddress(): ?string
  • getLocalAddress(): ?string

Resolves / closes #44
Refs reactphp/reactphp#199
Refs reactphp/socket#65
Refs reactphp/socket#68

@clue clue added this to the v0.6.0 milestone Feb 16, 2017
@clue
Copy link
Contributor Author

clue commented Feb 16, 2017

Note that this React\SocketClient\ConnectionInterface is somewhat similar to the React\Socket\ConnectionInterface (client context vs server context but otherwise identical methods).

It is believed this is a good thing currently, because we aim to avoid inter-package dependencies.

In the future, we will look into merging these into a single package again, in order to avoid confusion. This debate is best left out of this PR, be assured I'll file new tickets and link here if this ticket is accepted 👍

@clue clue merged commit 3473047 into reactphp-legacy:master Feb 16, 2017
@clue clue deleted the connection branch February 16, 2017 09:39
@clue
Copy link
Contributor Author

clue commented Feb 16, 2017

In the future, we will look into merging these into a single package again, in order to avoid confusion. This debate is best left out of this PR, be assured I'll file new tickets and link here if this ticket is accepted 👍

See #86 for future plans to merge this into the Socket component.

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.

Return Connection object which exposes remote address
3 participants