Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Jun 27, 2021

Conversation

mikeesouth
Copy link

  • 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 sefidgaran merged commit e0a44a7 into sefidgaran:master Jun 27, 2021
@sefidgaran
Copy link
Owner

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
Copy link
Author

@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
Copy link
Author

I will include this commit in the PR and it's not code that I'm very proud of: kattalo@669084f
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
Copy link
Author

New PR up, I hope that it solves the pub.dev issue: #7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants