search

firebase / firebase-ios-sdk

Firebase SDK for Apple App Development
https://firebase.google.com
Apache License 2.0
5.47k stars 1.43k forks source link

[FR] Migrate RTDB to use NSURLSession native WebSockets #5757

Open karimhm opened 4 years ago

karimhm commented 4 years ago

Feature proposal

Starting with iOS 13 NSURLSession now support WebSockets natively, currently Firebase uses FSRWebSocket.

Wouldn't be a good idea to rely on NSURLSession native WebSockets when available?.

charlotteliang commented 3 years ago

@karimhm Are you sure this is a Messaging issue? I don't think Messaging is using FSRWebSocket.

karimhm commented 3 years ago

@karimhm Are you sure this is a Messaging issue? I don't think Messaging is using FSRWebSocket.

Firebase Database seems the to be the only part that uses FSRWebSocket.

paulb777 commented 3 years ago

7656 added support for watchOS via WebSockets. We're hesitant to change the other platforms since we still support iOS 10 and would prefer not to have a version-specific implementation.

We're open to hearing a case otherwise though.

cc: @ryanwilson to check this explanation

ryanwilson commented 3 years ago

7656 added support for watchOS via WebSockets. We're hesitant to change the other platforms since we still support iOS 10 and would prefer not to have a version-specific implementation.

Yep, this was the primary reason. There's a worry we have subtle differences between the 2 and it could make debugging an issue. We haven't seen issues on iOS 13 or 14 using the SocketRocket implementation yet (outside of App Clips) so there doesn't seem to be an immediate rush. If things change in iOS 15 we'll re-evaluate, as well as try to get a solution that works in App Clips as well while trying to best determine a safe migration path.

We could potentially look into a flag that users could opt-in for testing purposes so it's a conscious decision for each dev, that could unblock App Clips but gets tricky with different package managers and would likely have to be a runtime flag.

All that being said, once iOS 13 is the minimum supported version I see no reason why we should keep the SocketRocket implementation.

mortenbekditlevsen commented 3 years ago

One issue with the version of SocketRocket that is used is that SOCKS proxying is not available and that makes VPN access (and debugging with tools like the Charles Proxy) impossible. I'd love to see the migration to NSURLSession some time! 😊

ryanwilson commented 3 years ago

One issue with the version of SocketRocket that is used is that SOCKS proxying is not available and that makes VPN access (and debugging with tools like the Charles Proxy) impossible. I'd love to see the migration to NSURLSession some time! 😊

That was something I was unaware of 😄 thanks @mortenbekditlevsen! Definitely something to re-evaluate for when we have some bandwidth to do so. Unfortunately we don't have a ton of great end-to-end tests for the socket code so it may require some groundwork there.

mortenbekditlevsen commented 3 years ago

The proxy functionality could also be obtained by upgrading SocketRocket, but I guess that also comes with a risk.