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.

Sign up for GitHub

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

Jump to bottom

hamed/refactor_ws_in_deriv_api #270

Open
wants to merge 1 commit into
base: flutter-version-3
Choose a base branch
from
Open

hamed/refactor_ws_in_deriv_api #270

wants to merge 1 commit into from

Conversation

hamed-rezaee
Copy link
Contributor

No description provided.

@mohammadt-deriv
Copy link
Contributor

mohammadt-deriv commented Jun 29, 2023 •
edited
Loading

web_socket_client is just a wrapper for web_socket_channel, with only 15 commits which is not stable yet.
Current version is dev: 0.1.0-dev.3
I think its better to grab idea from it and implement it our own.
btw, how is it better? do we have a metric or comparison for its advantages over our current implementation?

@hamed-rezaee
Copy link
Contributor Author

web_socket_client is just a wrapper for web_socket_channel, with only 15 commits which is not stable yet. Current version is dev: 0.1.0-dev.3 I think its better to grab idea from it and implement it our own. btw, how is it better? do we have a metric or comparison for its advantages over our current implementation?

As you said web_socket_client is a wrapper for web_socket_channel so all core functionalities of web_socket_channel is available.
Developer of this package is Felix Angelov and I checked the source code and the code quality is very good, and since Angelov is a well-known developer in the community, the package will be supported for sure. In the long term it has more benefits compared to developing a package by ourselves.
This package has 2 key features that could help us, first of all Reconnecting timer that allows making reconnect process automated, and the other one is Monitoring the Connection that gives us a stream of connection status and terminates dependency to connectivity_plus that is a package that has a lot of issues in different platforms.

if I want to summarize the benefits here they are:

  1. Package is written by the Bloc developer
  2. Reconnecting is supported
  3. Monitoring the Connection is supported
  4. Remove the connectivity_plus dependency
  5. No need to ping the server manually (this is not related to the new package, just a refactor)
  6. The source of truth for connectivity is only WS itself
  7. Reduce the complexity of the connection bloc

@hamed-rezaee
hamed/refactor_ws_in_deriv_api
0119c7e 
hamed/refactor_ws_in_deriv_api
@hamed-rezaee hamed-rezaee force-pushed the hamed/refactor_ws_in_deriv_api branch from 178b471 to 0119c7e Compare June 30, 2023 04:58
abedelaziz-deriv
abedelaziz-deriv reviewed Jul 3, 2023
View reviewed changes
lib/services/connection/api_manager/binary_api.dart
_webSocketChannel = ws.WebSocket(
uri,
pingInterval: const Duration(seconds: 1),
backoff: const ws.ConstantBackoff(Duration(seconds: 1)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about LinearBackoff ? Is it supported?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are looking for a way to connect asap, so I suppose ConstantBackoff is better in our case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ws.BinaryExponentialBackoff is the one should be used to avoid flooding the server.

pubspec.yaml
device_info_plus: ^8.1.0
package_info_plus: ^3.0.3
connectivity_plus: ^3.0.3
web_socket_client: ^0.1.0-dev.3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't use any dev packages. Especially with open bugs like this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abedelaziz-deriv this is just a POC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abedelaziz-deriv abedelaziz-deriv abedelaziz-deriv left review comments

@ramin-deriv ramin-deriv ramin-deriv left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hamed-rezaee @mohammadt-deriv @ramin-deriv @abedelaziz-deriv