Add optional write completion handler for emit's #1097
Conversation
|
This looks pretty good! Can you open this against the development branch? |
|
Great! I've changed the base branch |
|
Excellent, one thought I have though: Would it make sense to wrap the user provided callback with one that wraps and dispatches on the |
|
Something maybe like {[weak self] in
guard let this = self else { return }
this.handleQueue.async { /* call callback */ }
} |
|
Good idea – added! |
| @@ -295,8 +295,14 @@ open class SocketIOClient : NSObject, SocketIOClientSpec { | |||
| } | |||
|
|
|||
| func emit(_ data: [Any], ack: Int? = nil, binary: Bool = true, isAck: Bool = false, completion: (() -> ())? = nil) { | |||
| // wrap the completion handler so it always runs async via handlerQueue | |||
| let wrappedCompletion = {[weak self, completion] in | |||
There was a problem hiding this comment.
You shouldn't need to put completion in the capture list.
| /// - parameter items: The items to send with this event. Send an empty array to send no data. | ||
| /// - parameter completion: Callback called on transport write completion. | ||
| @objc | ||
| open func emit(_ event: String, with items: [Any], completion: @escaping () -> ()) { |
There was a problem hiding this comment.
One last thing, does it make sense to also add a nicer Swift version of this that uses variadics? I think it should be possible to define
open func emit(_ event: String, _ items: SocketData..., completion: () -> ()) { /* call to base version like the other methods */ }There was a problem hiding this comment.
Added a method for Swift. I also added the method signature to the SocketIOClientSpec, but not sure if that's needed?
| /// - parameter items: The items to send with this event. May be left out. | ||
| /// - parameter completion: Callback called on transport write completion. | ||
| open func emit(_ event: String, _ items: SocketData..., completion: @escaping () -> ()) { | ||
| emit([event] + items, completion: completion) |
There was a problem hiding this comment.
This needs to do the transformation from SocketData to something representable in JSON. See the other swift-centric methods.
There was a problem hiding this comment.
Ah yea of course, added.
|
Awesome, this looks good! I'll merge this in once 13.3.1 is merged into master (should be this week.) |
|
Great! Thanks for the quick review turnaround 👍 |
|
This PR introduces major crashes in productions, see #1096 (comment) I created a fork which reverts this PR and leads to a crash-free library: https://github.com/vonox7/socket.io-client-swift/commits/v-revert-completion-handler |
A first draft to add an optional completion handler for calls to emit, see #1096 for reasoning. I'm not a Swift expert so I'm quite likely doing something dumb here :)
Thanks to @AgentFeeble for providing a great starting point.
Fixes #1096