-
Notifications
You must be signed in to change notification settings - Fork 37
Conversation
src/dialer.js
Outdated
} | ||
} | ||
|
||
module.exports = Dialer |
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.
Let's avoid putting several classes in the same file, we already learned how that ends from Java.
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.
lol
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.
ok, just missing this then :)
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.
done
@@ -87,8 +87,7 @@ module.exports = function dial (swarm) { | |||
nextTransport(tKeys.shift()) | |||
|
|||
function nextTransport (key) { | |||
const multiaddrs = pi.multiaddrs.slice() | |||
swarm.transport.dial(key, multiaddrs, (err, conn) => { | |||
swarm.transport.dial(key, pi, (err, conn) => { |
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.
Hm, I intentionally had made swarm.transport.dial
be just multiaddr aware for simplicity, and so, it would only receive a multiaddr and not a peerInfo
swarm.transport.dial -> deals with dialing on a transport
swarm.dial -> does all the magic of upgrading the connection
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 is kind of a breaking change on the API, not a biggie because it is more of an internal that is just exposed for testing, however, I'm not sure if I see the advantage, could you clarify?
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 am doing the same thing that go does, that is limiting dials per peer. To do that I need to know the peerId otherwise I can't track dials per peer
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.
ok, we can keep it.
src/dialer.js
Outdated
if (token.cancel) { | ||
log('dialQueue:work:cancel') | ||
// clean up already done dials | ||
pull(pull.empty(), conn) |
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.
Just to make sure we don't loose track of this:
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.
No description provided.