Adding support for flutter for web and fixing bug with connectionToken in url query

sefidgaran / signalr_client

A Flutter SignalR Client for ASP.NET Core

https://pub.dev/packages/signalr_netcore

MIT License

71 stars 112 forks source link

Adding support for flutter for web and fixing bug with connectionToken in url query #6

Closed mikeesouth closed 3 years ago

mikeesouth commented 3 years ago

Added support for flutter for web by:
- Changing HttpClient from dart:io to http package with two separate clients for dart:io supporting devices and for browsers
- Replacing websocket from dart:io to web_socket_channel package that has support for web
- Using sse_client instead of w3c_event_source (not sure that this change was really necessary)
Fixed a bug with connection token that checked != null but the correct check should be == null

I haven't fixed/verified the tests. It wasn't as easy as "flutter test" so I didn't put time into it (yet). I can do that if the tests fail and if the author wants to merge this PR.

sefidgaran commented 3 years ago

Hi @mikeesouth , Thanks for the PR. I managed to review and merge this PR then I published it but the problem is in the pub.dev its says that its compatible only with Web. image below: 2D0D5A62-8DFA-423B-A4A8-158E16226DCC_4_5005_c Can you please check this issue. Thanks

mikeesouth commented 3 years ago

@sefidgaran Sorry for that, I had no idea that pub.dev would react like that. The import ofr http_client_browser is conditional and is only imported for web projects so the code fully supports iOS and Android even though pub.dev says it doesn't. I think we can just circumvent this by using "Client()" for both native and web projects, so remove the http_client_browser file. I can submit a new PR. But I'm curious what differs between this library and https://pub.dev/packages/signalr_core ? Maybe we should just switch to that library instead?

mikeesouth commented 3 years ago

I will include this commit in the PR and it's not code that I'm very proud of: https://github.com/kattalo/signalr_client/commit/669084fb9e6a9bd2fc8f1691a581fee2f796c602 It's just a quick fix to get rid of "completer already completed" exceptions that could occur in different parts of the code.

If you have good reasons to why this library should be used instead of https://pub.dev/packages/signalr_core, then I can put time into improving the completer code to be more robust.

Maybe it's better for you to just revert my previous PR and add web support yourself, if you still want to maintain this repo.

mikeesouth commented 3 years ago

New PR up, I hope that it solves the pub.dev issue: https://github.com/sefidgaran/signalr_client/pull/7