feat: add web support #73
Conversation
also remove unused old android code in chat_app example.
|
@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 #53 contained a good approach, so probably better build on top of it? |
|
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? |
|
Added support for setting headers on dart:io platforms. |
|
Thank you! 💪 I'll take a look during the upcoming week |
There was a problem hiding this comment.
@robert-virkus added some comments, could you please take a look?
example/chat_app/README.md
Outdated
| Also run Centrifugo in insecure client mode so it does not expect JWT token from client: | ||
|
|
||
| ```bash | ||
| ./centrifugo --config config.json --client_insecure --admin |
There was a problem hiding this comment.
Can we avoid suggesting client_insecure mode in examples, unfortunately this often leads to using client_insecure mode in production
There was a problem hiding this comment.
Ok, should I add the server config that matches the tokens in the code then?
Is it correct that a token generated with centrifugo gentoken do not expire?
There was a problem hiding this comment.
They expire (168h by default for gentoken), I think you can run go run main.go gentoken -u dart -t 604800000 - so token will be valid ~19 years - should be enough for examples. But just need to emphasise that in reality token TTL should be somewhat more reasonable - like 10-120 mins or so.
example/chat_app/README.md
Outdated
| ./centrifugo --config config.json --client_insecure --admin | ||
| ``` | ||
|
|
||
| Alternatively to running in insecure mode, you can regenerate the tokens in `lib/src/conf.dart` on your system. |
There was a problem hiding this comment.
So I think this should be default approach here. In general, examples may contain non-expiring tokens generated for the secret key used in the example. Would be awesome to mention that secret key should be changed and token expiration is recommended to have.
| } | ||
|
|
||
| Future? close() { | ||
| return _socket?.close(); | ||
| return _socket?.sink.close(); |
There was a problem hiding this comment.
Trying to understand where this change comes from? Does this work for non-web case also? Or do we have a bug currently in master branch?
There was a problem hiding this comment.
Previously _socket was a dart:io web socket, now it is an abstract web socket channel. Maybe it would be better to name it "_channel" or "_socketChanell"? So this is just a consequence of using the officially recommended web socket abstraction library now.
There was a problem hiding this comment.
Got it, thx. I think no real need to rename.
pubspec.yaml
Outdated
|
|
||
| environment: | ||
| sdk: '>=2.12.0 <3.0.0' | ||
| sdk: '>=2.18.0 <3.0.0' |
There was a problem hiding this comment.
Is it necessary to bump version here?
There was a problem hiding this comment.
No, it's not necessary but Dart 2.12 came out with Flutter 2.0 back in Q1 2021. With Dart 2.18 / Flutter 3.3 some useful modern language features like super-Parameters can be used. Based on your argument to move the ecosystem forward by not supporting older server versions, I assumed it to be preferable to use a more recent version. Compare https://docs.flutter.dev/development/tools/sdk/releases for details.
Let me know if you prefer to go back to Dart 2.12
There was a problem hiding this comment.
Let's use 2.12 for now if we are not using those features yet, it will be much simpler to make a new release and migrate, we can always bump later.
lib/src/transport.dart
Outdated
| @@ -1,8 +1,10 @@ | |||
| import 'dart:async'; | |||
| import 'dart:io'; | |||
| import 'dart:typed_data'; | |||
There was a problem hiding this comment.
Marked as unused in my vscode, maybe can be removed?
- downgrade dart dependency - improve code style - use supported linting
|
@FZambia how to proceed here? |
|
@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. |
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.