Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(IRC): add WebsocketKit support #17

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

kevinrpb
Copy link
Contributor

Hi there! Hope all is well 😄 I bring goodies!

I wanted to use this to deploy a small app to Docker. Unfortunately, I came across the same issue you had reported in FoundationNetworking: swiftlang/swift-corelibs-foundation#4730 (it was kind of amusing realizing both of us arrived at it lol). I even went ahead and compiled libcurl from source to enable the websocket support, but it turns out that it still produces errors...

So I went ahead and did the next best thing: add support for using WebsocketKit instead of URLSession (I think this may've already been in your plans, since it was a dependency).

I did it in such a way that switching between the two just involves changing a generic parameter, which I thought was neat. I also added the implementation in a different module so it's totally optional.

Some caveats is that I had to change some internal interfaces, and expose some. Let me know if you can come up with something else!

Finally, I have tested it in a simple client application, but I wouldn't say it is battle tested at all, so there's that. Although it's better than nothing, maybe?

Anyways, let me know if this makes sense to you to merge, or about any issues/comments you have about the implementation itself.

Cheers!

@kevinrpb kevinrpb changed the title Add WebsocketKit support for IRC feat(IRC): add WebsocketKit support Oct 31, 2024
@kevinrpb
Copy link
Contributor Author

kevinrpb commented Nov 2, 2024

Ah, seems like they fixed swiftlang/swift-corelibs-foundation#4730 so building libcurl from source may be a solution now. Although I'm not sure when the fix will actually ship ¯_(ツ)_/¯

@LosFarmosCTL
Copy link
Owner

LosFarmosCTL commented Nov 6, 2024

Hey, sorry for the late response, very busy with university so far sadly.

You guessed correctly, WebSocketKit für Linux has been a goal and the overall idea of your implementation looks good to me on first glance, it should definitely be an option for platforms where URLSession doesn't work, while the default still stays URLSession.

I'll take a closer look at the implementation this evening or tomorrow and get back to you again.

Ah, seems like they fixed swiftlang/swift-corelibs-foundation#4730 so building libcurl from source may be a solution now. Although I'm not sure when the fix will actually ship ¯_(ツ)_/¯

Even when the fix ships, building libcurl from source is imo not the ideal state, so I'd still like WebSocketKit to be an option.

@kevinrpb
Copy link
Contributor Author

kevinrpb commented Nov 6, 2024

Yeah, no worries, take as much as you need. If I need to use it I can point to my branch while this is in review 😄

@LosFarmosCTL
Copy link
Owner

Okay, so I've looked at all the changes now and in general the code looks pretty much good to go.

But I am not sure if extracting it to a separate target is a big benefit, I think it'd make more sense to just move all of the WebSocketTask/TaskProvider stuff into /Shared/WebSocket and conditionally include the WebSocketKit part in the build when running on Linux.
I'd like to use this implementation for EventSub as well, since that would have the same issues and just having a shared WebSocket implementation that can differentiate between the 2 platforms seems the simplest to me.

I do like that the separate target gives you the ability to use WebSocketKit on macOS/iOS/etc. as well, without carrying around the extra dependency if not needed and including it conditionally on Linux only would take that option away, but realistically I don't see a reason why you'd actually need it on Apple platforms.
The generics should definitely stay though, so you can switch to the URLSession version on Linux, if libcurl was compiled with WS support. The URLSession specific extension to TwitchIRCClient is really nice as well, the same thing for Linux, making WebSocketKit the 'default' there would make sense imo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants