search

endel / NativeWebSocket

🔌 WebSocket client for Unity - with no external dependencies (WebGL, Native, Android, iOS, UWP)
Other
1.13k stars 155 forks source link

Adds optional subprotocol #29

Closed Bradshaw closed 3 years ago

Bradshaw commented 3 years ago

I'm working on a legacy project that requires the subprotocol to be explicitly declared. This is possible in both the browser's WebSocket class, as well as C#'s System.Net.WebSockets system. This pull request adds optional constructors that allow passing a string or a list of strings with subprotocols defined.

The browser implementation passes the subprotocols as the second argument to new WebSocket()
The System.Net.WebSockets implementation adds the protocols via the ClientWebSocket.Options.AddSubProtocol() method

Bradshaw commented 3 years ago

For the #upm branch version of this pull request, see #30

endel commented 3 years ago

Hey @Bradshaw, thanks for your pull-request!

Can we make the second argument (subprotocols) optional?

Regarding the upm branch, there is a GitHub Workflow define here that automatically pushes into that branch (courtesy from @benukhanov)

endel commented 3 years ago

Nevermind, I just noticed we have 3 constructor alternatives now, that's great! Thanks!

endel commented 3 years ago

It seems the GitHub Workflow to update the UPM branch is not actually working, I've merged your PR into it for now. If Ben could check this it would be great! Otherwise I can have a look at it later this week. Cheers! 🙏

Bradshaw commented 3 years ago

Ah, yeah I put the subprotocol second because it matches with the browser's Websocket class that way.
I don't know if there's a way to write that in a single constructor, so I made 3 constructors to map to the different constructors available in the browser
It might not be the most appropriate way to do it, and having it as an optional third argument instead might fit your approach better 🤷

codingben commented 3 years ago

It seems the GitHub Workflow to update the UPM branch is not actually working, I've merged your PR into it for now. If Ben could check this it would be great! Otherwise I can have a look at it later this week. Cheers!

Yo, what is not working? I see there is PR #29 message here and latest commits here.

@Bradshaw Hi, by the way, FYI - we need to update the package version here.

Bradshaw commented 3 years ago

@benukhanov Noted, I updated the package version in #32 I also added a note recommending using Git tags and updating the documentation to point UPM at tagged versions rather than a branch

endel commented 3 years ago

Thanks a lot guys! ❤️ You're right @benukhanov, sorry, I misunderstood what was going on there!

Does this look good for the tagged version? @Bradshaw https://github.com/endel/NativeWebSocket/releases/tag/1.1.0

I've created this issue (https://github.com/endel/NativeWebSocket/issues/33) to keep track of adding a GH workflow for creating the tag, I'm probably gonna have some time for this during the weekend 🙏