Refactor websocket implementation directly into ApolloWebSocket #1906
Conversation
AnthonyMDev
requested review from
designatednerd and
calvincestari
and removed request for
designatednerd
| @@ -22,7 +21,7 @@ class StarWarsSubscriptionTests: XCTestCase { | |||
| connectionStartedExpectation = self.expectation(description: "Web socket connected") | |||
|
|
|||
| webSocketTransport = WebSocketTransport( | |||
| websocket: DefaultWebSocket( | |||
| websocket: WebSocket( | |||
There was a problem hiding this comment.
Do we not still want to abstract this out for when we offer URLSessionWebSocket compatibility in the future?
There was a problem hiding this comment.
It is still injected, you just need something that conforms to the WebSocketClient protocol. Just changed the name of the "Default" one to remove "Default".
I decided to hold off on moving the non-abstract parts of this into Apollo target until we do the networking layer refactor, because the API is going to likely change so drastically at that point anyways.
There was a problem hiding this comment.
OK sounds like a plan
|
This is nitpicking but it might be better to keep the Starscream files in their own sub-folder. If ApolloWebSocket grows in number of files it would be good to be able to isolate Starscream contributions. |
|
Agreed on throwing the Starscream files into a folder for clarity. |
|
I don’t think we should be naming the folder starscream. I don’t know why we are even moving them into a sub folder. I have already modified the starscream code modestly, and it’s now directly integrated as part of Apollo. |
|
@AnthonyMDev I disagree - we're taking it over, but I think it's really worth keeping "what is stuff we developed in-house" and "what is stuff we adopted from Starscream" very clearly delinated. |
|
I’m very okay with acknowledging the contributions of Starscream that we are deriving from, but going forward, this is no longer “Starscream” code. We have responsibility and ownership over the future or it. And it’s not included using any abstractions or DI or via a separate package dependency. |
|
@designatednerd Alright I hear you, but I’m not clear what actual value we get out of doing that. I guess I expect that as this library evolves, the lines between the two are going to get blurred even more. When we implement native Apple Websockets, we will still likely be using part of the Starscream derived code, but it will become even more heavily modified. I just don’t see the benefit of separation here. It feels like we are creating an artificial abstraction. Once we move the protocols and the WebSocketTransport into the base Apollo target (when we refactor the networking layer prior to 1.0), this Starscream derived code is going to become basically the entire target anyways. |
|
I’d be more inclined to accept a folder for “DefaultImplementation” or something. But I don’t think that it should be associated with Starscream in the project structure. |
|
I'm good with |
calvincestari
added
include-in-changelog
Due to dependency management conflicts in #1903, it has become easier for us to maintain our WebSockets as part of the
ApolloWebSocketstarget instead of an external dependency on a forked version of Starscream.This PR moves over the Starscream implementation directly into our target. I did a bit of refactoring and clean up to make the Starscream code make sense as a more integrated part of our websocket stack and to fix warnings for deprecated usage of unsafe Swift APIs.