centrifugal / centrifuge-dart

Dart (Flutter) client SDK for bidirectional communication with Centrifugo and Centrifuge-based server over WebSocket
https://pub.dartlang.org/packages/centrifuge
MIT License
102 stars 29 forks source link

feat: add web support #73

Closed robert-virkus closed 1 year ago

robert-virkus commented 1 year ago

also remove unused old android code in chat_app example.

This code adds web support using the web_socket_channel abstraction. The chat_app example supports now web, too.

The drawback of this approach is that no headers can be specified when opening the web socket. I am unsure about the real world implications here, at least in my (limited) experience this does not seem to matter.

FZambia commented 1 year ago

@robert-virkus hello, thanks!

Unfortunately we can't loose the possibility to set custom headers in non-browser environment. This change may break existing code without clear workaround. And in general, this is a good feature to have.

So the SDK should work the same way in non-browser, and we can comment that in browser env setting headers is not possible (noop).

I think https://github.com/centrifugal/centrifuge-dart/pull/53 contained a good approach, so probably better build on top of it?

robert-virkus commented 1 year ago

Thanks for the quick feedback! As you also say, the web implementation of #53 also ignores the header, compare https://github.com/centrifugal/centrifuge-dart/pull/53/files#diff-bfd41a418c915ac93a119f058c883ee52c0e52cfcdca3dbe699c95d279a3d3c7

I wonder if it might be better not to allow specifying headers at all, so to have the same set of capabilities across the web and dart:io-enabled platforms? What would be a real world scenario for requiring headers?

robert-virkus commented 1 year ago

Added support for setting headers on dart:io platforms.

FZambia commented 1 year ago

Thank you! 💪 I'll take a look during the upcoming week

robert-virkus commented 1 year ago

@FZambia how to proceed here?

FZambia commented 1 year ago

@robert-virkus hello, I think will be merged during the weekend, thanks so much. This is a great addition to the SDK, cant wait to proceed also.