Support websocket compression#339
Conversation
…comparable with the autobahn library itself.
| if let cfHeaders = CFHTTPMessageCopyAllHeaderFields(response) { | ||
| let headers = cfHeaders.takeRetainedValue() as NSDictionary | ||
| if let extensionHeader = headers[headerWSExtensionName as NSString] as? NSString { | ||
| let parts = extensionHeader.components(separatedBy: ";") |
There was a problem hiding this comment.
for readability what about moving this out to another method. There are already some uber methods on this class personally my preference on this is to keep method size down where it makes logical sense.
| // Created by Joseph Ross on 5/23/17. | ||
| // Copyright © 2017 Vluxe. All rights reserved. | ||
| // | ||
|
|
There was a problem hiding this comment.
Link to the spec here so that later if there is a bug one can quickly find the reference to follow: https://tools.ietf.org/html/rfc7692
|
Consider adding a short section to the readme for users. |
|
@hishnash: Thanks for the great feedback. I've made all the changes you suggested. |
…sed on PR feedback.
…inition. This is less verbose and less error-prone.
|
When is this support going to be available ? |
|
+1 Also interested in this |
|
@daltoniam: Is there anything I can do to help this PR land? Would it be better if I made compression disabled by default, and those who want to use it have to opt-in? |
|
@joejune thank you much for this, it looks great. Also sorry for the delay, got distracted with life and WWDC. Love the tests, README updates, autobahn tests, very through much appreciated. |
|
Did you test this with the Swift Package Manager? I have a feeling it might not like that you've included zlib outside of Sources. |
|
ah right, package managers 😄. I have not done that yet, might have to fix that. |
|
@nuclearace is there a way to test the package manager without have to create a new tag? |
|
Uh, if you create an app that has Starscream as a dependency and then put Starscream into edit mode. |
|
Actually I take that back, that wouldn't really help test if the dependency itself is right. I can't remember if they've added the ability to point to a branch |
|
Ok cool. Yeah, think it is going to be in Swift 4. |
|
I guess I'll just create a test tag and work off of that ( |
|
Found something I think might help: |
|
@nuclearace I figured it out. I updated the 2.1.0 to build properly again. |
|
Thanks for getting this merged in. I haven't worked with Swift Package Manager yet, so thanks for taking care of that aspect. |
6 participants
Fixes #169.
Using the Autobahn test suite, I confirmed that these changes perform comparably with the Autobahn library itself. See attached report:
autobahn_test.html.zip