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

Replace the ws read interface with event callback. #1

Closed
wants to merge 2 commits into from

Conversation

c98
Copy link

@c98 c98 commented Jan 1, 2017

This is a feature pull request.

the key concept is that websocket use onMessageHandler instead of readStringMessage() or readBinaryMessage().

The problem which I occured is that I have to write the-read-logic(i.e. readStringMessage) in every sendStringMessage completion closure. The example is confusing(e.g. handleSession in the README.md), and in some complex case, this solution seems not good.

In this pull request, I moved the-read-logic into the websocket internal inspired by event-driven model. Here is the logic:

websocket

Now, we can use websocket simply, for example:

class AccessLayerHandler: WebSocketSessionHandler {
	// The name of the super-protocol we implement.
	// This is optional, but it should match whatever the client-side WebSocket is initialized with.
	let socketProtocol: String? = nil

	func handleSession(request: HTTPRequest, socket: WebSocket) {
		socket.onMessageHandler = {
			message, op, fin in
			guard let message = message else {
				socket.close()
				return
			}
			// TODO: handle the message
			// e.g. Echo this message back to client.
			socket.sendStringMessage(string: "echo", final: true) {}
		}
	}
}

That's it.

kjessup pushed a commit that referenced this pull request Jan 20, 2017
Removed minor version pinning of Perfect-HTTP
@kjessup
Copy link
Member

kjessup commented Jan 20, 2017

Thank you for the PR. This change removes existing API and would cause breakage were it to be tagged under the current major version #. Is it possible to have both the older/existing API and the new method on top? Then nothing would break and the older functions can eventually be deprecated.

@c98
Copy link
Author

c98 commented Feb 3, 2017

@kjessup Thank you for your reply, and I think it over again.
imo, the existing API readStringMessage or readBinaryMessage conflicts with the new API onMessageHandler, unfortunately there is no way to keep both. :(

@saroar
Copy link

saroar commented Oct 22, 2017

did you decide it will be marge in master branch?

@c98
Copy link
Author

c98 commented Oct 27, 2017

@saroar I will close this PR.

@c98 c98 closed this Oct 27, 2017
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.

3 participants