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

Merge SocketClient component into this component #87

Merged
merged 197 commits into from
Apr 10, 2017
Merged

Conversation

clue
Copy link
Member

@clue clue commented Apr 4, 2017

Here's the existing situation:

  • This Socket component: Async, streaming plaintext TCP/IP and secure TLS socket server for ReactPHP
  • SocketClient component: Async, streaming plaintext TCP/IP and secure TLS socket client for ReactPHP

This is not exactly ideal, they share quite a bit of common code and actually have a dependency on one-another for their test suite. Also, I believe this situation may be a bit confusing for consumers of this package. In particular our Datagram component provides both client and server side for datagram sockets (UDP).

This PR simply merges the existing SocketClient component into this package. This is done as-is, with only the namespace changed to React\Socket. This merges the existing history as to preserve original authorship (git blame etc.) into this component and only changes what necessarily has to be changed (merge conflicts existed pretty much only in meta files such as the README).

If you want to review, only the last three commits are part of this merging effort, anything else is actually part of the SocketClient history (which can be verified by checking the commit hashes).

Resolves / closes #74

igorw and others added 30 commits January 15, 2013 12:32
…et-client

* robinvdvleuten/socket-client:
  Make the transport method of the socket variable.
@clue clue added this to the v0.7.0 milestone Apr 4, 2017
@clue
Copy link
Member Author

clue commented Apr 4, 2017

Updated to work around an issue in legacy HHVM < 3.8 :shipit:

README.md Outdated
@@ -1,17 +1,28 @@
# Socket Component
# react/socket
Copy link
Member

Choose a reason for hiding this comment

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

Tbh, i don't like using the lowercase composer handle as the readme title :(
My personal preference would be just Socket. I'd even prefer the old title over react/socket.

README.md Outdated
@@ -1,17 +1,28 @@
# Socket Component
# react/socket
Copy link
Member

Choose a reason for hiding this comment

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

Agreeing with @jsor here, the old Socket Component looks better. Some random other ideas for it:

  • Client and Server Socket Component
  • Socket Component (Client and Server)
  • Component providing Client and Server
  • Client/Server Socket Component

Copy link
Member

Choose a reason for hiding this comment

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

Just to be clear, my favorite is just Socket 😃

Copy link
Member

Choose a reason for hiding this comment

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

Definitely

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @jsor and @WyriHaximus, I think you both made a very valid point and it doesn't make much sense to change this as part of this PR in the first place 👍

I've updated this to just "Socket" (originally "Socket Component"), because the paragraph below uses the words "server and client component" :shipit:

@clue clue merged commit c15f458 into reactphp:master Apr 10, 2017
@clue clue deleted the merge branch April 10, 2017 10:40
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.

Merge SocketClient component into this component