feat: connectedMultiaddr #49
Conversation
src/index.js
Outdated
| constructor (peerId) { | ||
| assert(peerId, 'Missing peerId. Use Peer.create(cb) to create one') | ||
|
|
||
| this.id = peerId |
There was a problem hiding this comment.
should we do this.peerId for consistency everywhere?
There was a problem hiding this comment.
no, please not I need to type that so often, and it's already clear that this things i a peer, as it is a peerInfo
src/index.js
Outdated
| this._observedMultiaddrs = [] | ||
| this._connectedMultiaddr = undefined | ||
|
|
||
| this.multiaddr = {} |
There was a problem hiding this comment.
Let's make all methods first class, new names welcome
src/index.js
Outdated
| this.multiaddr.add(addr) | ||
| observedMultiaddrs.splice(i, 1) | ||
| check = true | ||
| this.multiaddr.replace = (existing, fresh) => { |
There was a problem hiding this comment.
Maybe break this into two functions "replace and merge"?
src/index.js
Outdated
| fresh.forEach((m) => { | ||
| this.multiaddr.add(m) | ||
| }) | ||
| setConnectedMultiaddr (ma) { |
There was a problem hiding this comment.
not super happy with this name
src/index.js
Outdated
| this.id = peerId | ||
| this.multiaddrs = new MultiaddrSet() | ||
| this.protocols = new Set() | ||
| this._connectedMultiaddr = undefined |
There was a problem hiding this comment.
Are we sure we are ever only connect on one addr?
There was a problem hiding this comment.
otherwise this could just be another multiaddrset
There was a problem hiding this comment.
Yes, there might be situations where we are dialing on several or in between a upgrade, but that is swarm magic, for the rest of js-ipfs, it is always just one.
src/multiaddr-set.js
Outdated
| return this._multiaddrs | ||
| } | ||
|
|
||
| get length () { |
There was a problem hiding this comment.
I am torn on this one, Set uses .size and we are calling this a set..so not sure
There was a problem hiding this comment.
ah. I actually used your gist as a template. size or lenght then?
There was a problem hiding this comment.
Don't be torn, we will solve it :)
src/multiaddr-set.js
Outdated
| } | ||
|
|
||
| toArray () { | ||
| return this._multiaddrs |
There was a problem hiding this comment.
let's return a copy to avoid accidental changes, this._multiaddrs.slice()
src/multiaddr-set.js
Outdated
| add (ma) { | ||
| ma = ensureMultiaddr(ma) | ||
|
|
||
| if (!this.multiaddrHas(ma)) { |
There was a problem hiding this comment.
multiaddrHas is not a function ;)
src/multiaddr-set.js
Outdated
| } | ||
|
|
||
| // to prevent multiaddr explosion due to identify | ||
| addSafe (ma) { |
There was a problem hiding this comment.
why do we need this now that we already check for uniqueness on the regular add?
There was a problem hiding this comment.
Multiaddr explosion is when you dial to a bunch of nodes and every node gives you a different observed address and you start storing them all to share with other peers. This seems like a good idea until you realize that most of those addresses are unique to the subnet that peer is in and so, they are completely worthless for all the other peers. This method is exclusively used by identify.
There was a problem hiding this comment.
might be good to add that explanation into here :)
daviddias
force-pushed
the
feat/advanced-peer-book
branch
from
6d0b768
to
5840f9d
Compare
src/multiaddr-set.js
Outdated
| } | ||
|
|
||
| // to prevent multiaddr explosion due to identify | ||
| addSafe (ma) { |
There was a problem hiding this comment.
might be good to add that explanation into here :)
src/multiaddr-set.js
Outdated
| const ensureMultiaddr = require('./utils').ensureMultiaddr | ||
|
|
||
| // Because JavaScript doesn't let you overload the compare in Set().. | ||
| class MultiaddrSet { |
There was a problem hiding this comment.
Can I get some nice jsdoc comments while we are at it? :)
There was a problem hiding this comment.
Will do that on the
src/multiaddr-set.js
Outdated
| this.multiaddr.add(ma) | ||
| this._observedMultiaddrs.splice(i, 1) | ||
| check = true | ||
| } |
There was a problem hiding this comment.
this never returns true, so you could just use forEach if you want to do it on all of them, otherwise just do const check = multiaddrs.some() and return true when matching.
README.md
Outdated
| - [`.multiaddrs.add(addr)`](#multiaddraddaddr) | ||
| - [`.multiaddrs.addSafe(addr)`](#multiaddraddsafeaddr) | ||
| - [`.multiaddrs.forEach(fn)`](#multiaddrforeachfn) | ||
| - [`.multaiddrs.size()`]((#multiaddrsize) |
There was a problem hiding this comment.
size is a getter not a method
test/index.spec.js
Outdated
|
|
||
| it('.disconnect', () => { | ||
| pi.disconnect('/ip4/127.0.0.1') | ||
| expect(pi.isConnected()).to.equal(false) |
There was a problem hiding this comment.
I am missing some tests for toArray, forEach, clear and has
No description provided.