-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: added WebSocket support in AtLookup #709
Conversation
@@ -240,22 +243,24 @@ class AtLookupImpl implements AtLookUp { | |||
|
|||
Future<void> createConnection() async { | |||
if (!isConnectionAvailable()) { | |||
if (_connection != null) { | |||
if (connection != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see connection and _connection both..can we use just _connection while referring the var inside AtLookUpImpl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated be8058b
@@ -6,9 +6,12 @@ library at_lookup; | |||
export 'src/at_lookup.dart'; | |||
export 'src/at_lookup_impl.dart'; | |||
export 'src/connection/outbound_connection.dart'; | |||
export 'src/connection/outbound_websocket_connection.dart'; | |||
export 'src/connection/outbound_connection_impl.dart'; | |||
export 'src/exception/at_lookup_exception.dart'; | |||
export 'src/monitor_client.dart'; | |||
export 'src/cache/secondary_address_finder.dart'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we have modified SecondaryUrlFinder constructor and exported this class, the change should be treated as breaking and have a major release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated be8058b
.atLookupSocketListenerFactory(outboundConnection); | ||
|
||
// Set the connection type in `_webSocketConnection` or `_connection` | ||
if (underlying is WebSocket) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this type casting has no impact on the code since we are not accessing any attributes specific to OutboundWebSocketConnection or OutboundConnection
If _connection is not set before, we can just have
_connection = outboundConnection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated [be8058b]
export 'src/connection/outbound_connection_impl.dart'; | ||
export 'src/exception/at_lookup_exception.dart'; | ||
export 'src/monitor_client.dart'; | ||
export 'src/cache/secondary_address_finder.dart'; | ||
export 'src/cache/cacheable_secondary_address_finder.dart'; | ||
export 'src/util/secure_socket_util.dart'; | ||
export 'src/connection/outbound_websocket_connection_impl.dart'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid exporting the implementation classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
export 'src/connection/outbound_connection_impl.dart'; | ||
export 'src/exception/at_lookup_exception.dart'; | ||
export 'src/monitor_client.dart'; | ||
export 'src/cache/secondary_address_finder.dart'; | ||
export 'src/cache/cacheable_secondary_address_finder.dart'; | ||
export 'src/util/secure_socket_util.dart'; | ||
// export 'src/connection/outbound_websocket_connection_impl.dart'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest removing the line instead of commenting it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, My Bad!! Removed commented line
@@ -661,7 +684,7 @@ class AtLookupImpl implements AtLookUp { | |||
Future<void> _sendCommand(String command) async { | |||
await createConnection(); | |||
logger.finer('SENDING: $command'); | |||
await _connection!.write(command); | |||
_connection!.write(command); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await is removed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think await is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I verified the code and "write" operation on the socket does not return a future. Hence suggested to remove await.
serverSide: false, | ||
); | ||
|
||
print('WebSocket connection established'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this print and replace with logger
Opened a new PR with the refactored code. Hence closing this PR |
- What I did
Implemented WebSocket support by introducing a new connection factory structure to support both SecureSocket and WebSocket connections. This update allows AtLookupImpl to dynamically choose between secure socket and WebSocket connections, enhancing flexibility and enabling WebSocket-based communication when required.
- How I did it
- How to verify it
- Description for the changelog
add WebSocket support in AtLookupImpl