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 Codable Messages #103

Closed
wants to merge 3 commits into from
Closed

Support Codable Messages #103

wants to merge 3 commits into from

Conversation

dioKaratzas
Copy link

@dioKaratzas dioKaratzas commented Jan 10, 2022 •
edited
Loading

(#103, fixes #62).

Many Thanks to @MihaelIsaev for the inspiration!

Those changes adds support to send Codable messages and receive events in special JSON format:

{ "event": "<event name>" }
or
{ "event": "<event name>", "data": <anything> }

The event data will be decoded to the provided Type.

Here is a demo:

struct WebSocketController: RouteCollection {
	func boot(routes: RoutesBuilder) throws {
		routes.webSocket("chat") { req, ws in
			ws.onEvent("hello", helloHandler)

			ws.onEvent("bye", Bye.self) { ws, data in
				print("Bye \(data.firstName) \(data.lastName)")
				ws.send(Response(status: "Bye bye!"))
			}

			ws.onText { ws, text in
				print(text)
			}

			ws.onEvent("stop") { ws in
				// stop event without data
			}

			ws.onEvent("start", startHandler)
		}
	}
}

// MARK: - WebSocket Event Handlers
extension WebSocketController {
	func helloHandler(webSocket: WebSocket, data: Hello) {
		print("Hello \(data.firstName) \(data.lastName)")
		webSocket.send(Response(status: "Hey There \(data.firstName) \(data.lastName)"), type: .binary)
	}

	func startHandler(webSocket: WebSocket) {
		// start event
	}
}

// MARK: - Codables
struct Hello: Codable {
	let firstName, lastName: String
}

struct Bye: Codable {
	let firstName, lastName: String
}

struct Response: Codable {
	let status: String
}

The code will become much cleaner as you can give on events a handler!

@dioKaratzas dioKaratzas marked this pull request as ready for review January 10, 2022 13:40
@dioKaratzas dioKaratzas changed the title Support Codable Messages closes #62 Support Codable Messages Closes #62 Jan 10, 2022
@dioKaratzas dioKaratzas changed the title Support Codable Messages Closes #62 Support Codable Messages Jan 10, 2022
@dioKaratzas dioKaratzas changed the title Support Codable Messages Support Codable Messages Closes #62 Jan 10, 2022
@dioKaratzas dioKaratzas changed the title Support Codable Messages Closes #62 Support Codable Messages Jan 10, 2022
0xTim
0xTim requested changes Jan 13, 2022
View reviewed changes
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

@dnKaratzas I've added the logging APIs needed to be able to inject it in. A few things to discuss but thanks for the PR!

Sources/WebSocketKit/WebSocket.swift Outdated Show resolved Hide resolved
Sources/WebSocketKit/WebSocket.swift Outdated Show resolved Hide resolved
Sources/WebSocketKit/Exports.swift Outdated Show resolved Hide resolved
Sources/WebSocketKit/WebSocket.swift Outdated Show resolved Hide resolved
Sources/WebSocketKit/WebSocket.swift Outdated Show resolved Hide resolved
Sources/WebSocketKit/WebSocket.swift Outdated Show resolved Hide resolved
Sources/WebSocketKit/WebSocket.swift Outdated Show resolved Hide resolved
Sources/WebSocketKit/WebSocket.swift Outdated Show resolved Hide resolved
Tests/WebSocketKitTests/AsyncWebSocketKitTests.swift Outdated Show resolved Hide resolved
0xTim
0xTim previously approved these changes Jan 18, 2022
View reviewed changes
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

LGTM!

@dioKaratzas dioKaratzas mentioned this pull request Jan 18, 2022
Closed
Joannis
Joannis requested changes Jan 19, 2022
View reviewed changes
Copy link
Member

@Joannis Joannis left a comment

Choose a reason for hiding this comment

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

I love this! But I want to have a good look at this before it gets merged.

First of all, sending and receiving JSON as text is a very common and valid use case. But I'd prefer that this API doesn't imply that 'binary' means 'UTF-8 encoded JSON Codable types'. If I'm using binary, which doesn't happen much anyways, I'm probably not sending JSON Codable types. I'm using a binary format of something.

I would love to not have all unnecessary APIs added. Specifically the (binary) WebSocketSendType. But also the event types, because I don't think there's a 'standard' for websocket events anywhere. And if there is, this should be documented as being said standard.

Also, the implementation assumes that an event's contents are optional. For me, that would never be the case, unless I'd mark the event as WebSocketevent<MyEvent?>.

In Short:

I love Codable messages, not so fond of adding the rest to something like WebSocketKit.
And while sending Codable data is supported, receiving Codable data only works on the Event type. I'd prefer seeing an onJSONText(MyEvent.self) { ws, event in .. }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Joannis Joannis Joannis requested changes

@0xTim 0xTim 0xTim left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Codable messages
3 participants
@dioKaratzas @Joannis @0xTim