Fix dialer.connectToPeer to throw a clean error #673
Conversation
There was a problem hiding this comment.
@dthorpe Thanks for having the time to fix this.
I left some feedback
| @@ -96,6 +96,14 @@ describe('Dialing (direct, TCP)', () => { | |||
| .and.to.have.nested.property('._errors[0].code', ErrorCodes.ERR_TRANSPORT_UNAVAILABLE) | |||
| }) | |||
|
|
|||
| it('should fail appropriately if peer has no addresses', async () => { | |||
| const dialer = new Dialer({ transportManager: localTM, peerStore }) | |||
| const peer = new PeerId(Buffer.from('QmfEa1LrSt2R7KiZ31yPo3TT8ipfyDfrMRkaQix3rcXeTo')) | |||
There was a problem hiding this comment.
Can you use the createPeerId function already required to create the PeerId? We try to use that because it creates a PeerId from a JSON which already have everything computed and the tests will be faster to run.
| @@ -111,6 +111,7 @@ | |||
| "libp2p-tcp": "^0.14.1", | |||
| "libp2p-webrtc-star": "^0.18.0", | |||
| "libp2p-websockets": "^0.13.1", | |||
| "mocha": "^8.0.1", | |||
There was a problem hiding this comment.
What is the reason to require mocha here?
There was a problem hiding this comment.
seemed to be required to run the unit tests in intellij. 🤷
There was a problem hiding this comment.
This is strange. I do not have intellij to test this out, but running the tests in the CLI does not require mocha as we are not using it
| @@ -70,7 +70,7 @@ class Dialer { | |||
| async connectToPeer (peer, options = {}) { | |||
| const dialTarget = this._createDialTarget(peer) | |||
|
|
|||
| if (!dialTarget.addrs.length) { | |||
| if (!dialTarget.addrs || !dialTarget.addrs.length) { | |||
There was a problem hiding this comment.
What do you think on doing the following in the this._createDialTarget?
let addrs = this.peerStore.addressBook.getMultiaddrsForPeer(id) || []I think it would be cleaner.
|
@dthorpe will you be able to finish this review, or should I take it? |
when dialTarget.addrs is undefined
Fixes #672