-
Notifications
You must be signed in to change notification settings - Fork 37
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
refactor(dht): Do not store ConnectionType
#2049
base: main
Are you sure you want to change the base?
Conversation
ConnectionType
ConnectionType
4b10d02
to
06a11ad
Compare
06a11ad
to
0ed9282
Compare
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.
Sounds to me (on the surface level) like we should keep the logging statements and tests assertions related to connectionType. But instead of storing connectionType we can infer it statically with a helper function?
@@ -362,7 +362,7 @@ export class ConnectionManager extends EventEmitter<TransportEvents> implements | |||
private onConnected(connection: ManagedConnection) { | |||
const peerDescriptor = connection.getPeerDescriptor()! | |||
this.emit('connected', peerDescriptor) | |||
logger.trace(getNodeIdFromPeerDescriptor(peerDescriptor) + ' onConnected() ' + connection.connectionType) | |||
logger.trace(getNodeIdFromPeerDescriptor(peerDescriptor) + ' onConnected()') |
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 would keep the connectionType in the logging statements when it is relevant.
expect((node1.getTransport() as ConnectionManager).getConnection(node2.getLocalPeerDescriptor())!.connectionType) | ||
.toEqual(ConnectionType.WEBRTC) | ||
expect((node2.getTransport() as ConnectionManager).getConnection(node1.getLocalPeerDescriptor())!.connectionType) |
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.
These assertions related to connectionType are important to validate?
We don't need to store a
connectionType
value for eachManagedConnection
instance as the information can be queried by calling theexpectedConnectionType
helper function. The helper was introduced in #2043.Removed obsolete
ConnectionType#SIMULATOR_*
enum values.Future improvements
ConnectionType
could be string literals instead of enum values as we prefer those nowadays.