Remove dependency on starscream in favor of URLSession

[pokryfka]

Starting from iOS13 URLSession supports web sockets.

Please consider removing dependency on starscream

[shaps80]

Ahh I was wondering why the requirement on Starscream +1 from me too 👍

[shaps80]

I'm unclear about something on this one.
I've been going through the Issues, Discussions and PRs and I found #40 as well as #15

AFAICT (and I may be misreading) I was expecting to find this already done. I was wondering if perhaps @maticzav could explain why the result wasn't Apple's web sockets here?

[yonaskolb]

Looks like this was the PR that added Starscream #70

[shaps80]

@yonaskolb ah thanks I couldn't find it!
Unfortunately I don't see a mention as to why though ¯_(ツ)_/¯

[shaps80]

I've since discussed briefly with @maticzav and he believes this may have been down to a reliability issue when he attempted to use URLSession's implementation.

That being said, I'd love to re-explore this and would be open to PRs that switch to URLSesssion Socket APIs.

AFAICT the issues seen previously may have been related to earlier iOS implementations which may no longer be valid reasons since we've dropped support for iOS < 15 and next week will likely bring the rest inline:

iOS 15+
tvOS 15+
macOS 11+
watchOS 8+

@pokryfka if you'd be interested in contributing I'd be happy to review and test and see if we can drop Starscream altogether 👍

[pokryfka]

I will have a look and can do it if I manage to run the test server in this repository.

For the record, I have been using URLSession websocket with Binance (but not with GraphQL subscription) in production for quite some time now with no issues.

[shaps80]

Yeah me too. I mentioned it to @maticzav the other day that I've actually been using the socket implementation for sometime myself without issue.

But to be honest he can't remember exactly what the result was and why. And it could have been I guess related specifically to graphQL subscriptions, but intuition tells me we should give this another stab anyway

👍

I'll ensure this is scheduled, just let me know if you plan on starting on this, and if so I won't make an attempt myself 👍

[pokryfka]

I made POC, see #178 #180.

Happy to discuss and help with it but I dont think I am the right person to finish it as I use different transport protocols in my projects (see #158).

Note that as of now there is only 1 unit test (which passes).

[pokryfka]

I think the refactoring to use URLSession is finished, see comments in #180 and additional tests in #181

However, even though - as of now - it does not change the public interface and functionality of GraphQLWebSocket it IS a major change and personally I dont think should be part of v5.

@shaps80 could you consider creating an integration branch for v6 which would allow to merge approved but experimental and braking changes.

Personally I can see following roadmap here:

• replace Starscream with URLSession web socket
• review public interface and add tests cases, may introduce minor breaking changes (for example close() method does not work as documented, IMO it should also not allow sending server specific reserved close codes)
• add support for different transport protocols, see Add support for AWS AppSync #158
• make GraphQLWebSocket thread safe, add support for structured concurrency (async functions, async stream), may introduce braking changes

[shaps80]

@pokryfka I agree re: v6 release for socket updates of this magnitude. In fact it was already scheduled on that project, but I thought perhaps you were working on this because you needed it and I wanted to remain open to an approach that could enable that.

I will however move it to v6 but I think one problem here is that we need to remain coordinated particularly in regards to v6 work – none of which I've yet prioritised – so I'd perhaps suggest you hold off on anything not in the "todo" list on the project board. This will ensure we are working towards the same goals and that work is going in within the context of other changes.

I'll cover v5 vs v6 below, but just to address your "roadmap" above. In general I agree but this assumes work in isolation (sockets) whereas v6 will likely introduce many other changes that will require additional work (likely before this work can be properly addressed) that will inevitably effect some of the approach taken here (and in other work).

Hence the coordinated effort requirement.

---

I haven't had enough time to do this yet, so you're kinda jumping the gun sorry lol, but I have been making progress creating some initial Discussions and prioritising v5 work to ensure we have support of the existing libraries.

My larger goal however is more ambitious and the primary reason for wanting to co-maintain the project. I am planning to outline a larger strategy (through the use of smaller discussions) that will give us an intended design for v6.

Its a major update but more importantly a huge opportunity to bring some more substantial updates to the library.

While I agree, your socket implementation is relatively simple (in the context of the current codebase) I don't feel (yet) that it would be appropriate to merge that even into a v6 branch (which is planned of course), given it is still confined to some of the existing constraints I want to explore removing (e.g. async/await vs completion handlers for one)

I hope that helps provide some additional context, I had planned on formalising this a little more, but I've not had time yet and have been sick most of this week as well (not to mention other work/family responsibilities haha).

I absolutely love that you're so involved, I just prefer a more coordinated approach to ensure we're aligned with larger design goals before merging in changes that kinda work "today".

---

To sum up, here's my suggestion. I feel that this is a nice clean implementation of how we can handle sockets. That's amazing so thank you!

However, I think we should keep this more as a reference (for now) for some future work, once more of the infrastructural changes have been designed and likely included in a v6 branch, providing a better foundation for building on.

I have already started a discussion on Concurrency (#174) that I'd love your input and contribution on, especially regarding real use-cases you've uncovered and the limitations/pain-points you've encountered.

These will be extremely beneficial in helping us determine what's possible and potential approaches to solving those problems, while ensuring we're solving the "right problems".

Again, I'm so greatly for your input and contributions, and I hope this better clarifies my intent and gives us a greater opportunity to work well together.

Speak soon, enjoy your break! 🎉

[pokryfka]

While I agree, your socket implementation is relatively simple (in the context of the current codebase) I don't feel (yet) that it would be appropriate to merge that even into a v6 branch (which is planned of course), given it is still confined to some of the existing constraints I want to explore removing (e.g. async/await vs completion handlers for one)

That's why, personally, I think it makes sense to break it down to steps like:

• updating test coverage to better know how it works at the moment
• keeping the functionality and interface intact and drop the dependency on Starscream
• updating functionality and/or internals, possibly introducing breaking changes

Trying to do all together at the same time is a more chaotic/risky approach.

Anyway. as mentioned earlier, I am not in a rush, looking forward to see your changes to the project and happy to see that the library is not dead :-D

---

the existing constraints I want to explore removing (e.g. async/await vs completion handlers for one)

on that note, GraphQL subscription is where Combine still have advantage over async streams because it can ha multiple consumers, see apple/swift-async-algorithms#110 (comment)

[shaps80]

on that note, GraphQL subscription is where Combine still have advantage over async streams because it can ha multiple consumers, see apple/swift-async-algorithms#110 (comment)

That's a fair point but I feel its likely not the common use case and we should optimise for those imo.

Note: there will likely be a plan to include Combine (via an add-on package) that would potentially enable this kind of thing as a "backup" for those case I think.

[shaps80]

Mat and I have decided to focus our efforts on v6 which will bring URLSession based socket support anyway. So this is no longer planned for v5.