Add a WebSocket client upgrader.#1038
Add a WebSocket client upgrader.#1038Lukasa merged 10 commits intoapple:masterfrom tigerpixel:websocket-client-upgrader
Conversation
Motivation: There is a client protocol upgrader but, unlike the server protocol upgrader, it does not have a WebSocket protocol as part of the project. Modifications: Made the magic WebSocket GUID public to the WebSockets project. Added a NIOWebSocketClientUpgrader. Added tests for the upgrader. Updated the Linux test script files. Result: The project now has a WebSocket client upgrader to match the WebSocket server upgrader.
|
Can one of the admins verify this patch? |
5 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
Motivation: To ensure that the test are readable. Modifications: Slight changes to web socket client and client upgrader tests. Result: Neater WebSocket and client upgrader tests.
weissi
left a comment
There was a problem hiding this comment.
this looks really good, just some minor suggestions
… and server upgraders. Motivation: To increase the legibility of the server and client upgrade classes. Modifications: Slight changes to web socket client and server upgraders. Result: Easier to read WebSocket client and server upgrade classes.
…ixel/swift-nio into websocket-client-upgrader
|
@swift-nio-bot test this please |
weissi
left a comment
There was a problem hiding this comment.
thank you very much @tigerpixel! looks great
|
@swift-nio-bot test this please |
| /// This upgrader assumes that the `HTTPClientUpgradeHandler` will create and send the upgrade request. | ||
| /// This upgrader also assumes that the `HTTPClientUpgradeHandler` will appropriately mutate the | ||
| /// pipeline to remove the HTTP `ChannelHandler`s. | ||
| public final class NIOWebClientSocketUpgrader: NIOHTTPClientProtocolUpgrader { |
There was a problem hiding this comment.
Is this a typo?
- NIOWebClientSocketUpgrader
+ NIOWebSocketClientUpgraderThere was a problem hiding this comment.
@tanner0101 omg, yes, this is a typo :|. Mind making PR to rename (alongside a deprecated typealias to have the NIOWebClientSocketUpgrader still valid to not break SemVer).
There was a problem hiding this comment.
@tanner0101 @weissi Sorry, my bad. Definitely is a typo. I am not sure how that happened. Changing this will also require a change to the WebSocketClientDemo to use the updated class name rather than the old via the type alias.
I am happy to make a PR for one or both of these if you would rather not do it. Let me know.
There was a problem hiding this comment.
@tigerpixel don't apologise, we all missed it :). We can land these changes in either 1 or 2 PRs, just need to make sure we retain a (deprecated) typealias so the typo'd name stays usable (for SemVer compatibility). NIO's public API breakage checker should however catch any mistakes that we might make.
Add a WebSocket client upgrader.
Motivation:
There is a client protocol upgrader but, unlike the server protocol upgrader, it does not have a WebSocket protocol as part of the project.
Modifications:
Made the magic WebSocket GUID public to the WebSockets project.
Added a NIOWebSocketClientUpgrader.
Added tests for the upgrader.
Updated the Linux test script files.
Result:
The project now has a WebSocket client upgrader to match the WebSocket server upgrader.