-
Notifications
You must be signed in to change notification settings - Fork 60
Conversation
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.
Thanks for taking this @achingbrain! 🎉 Also a lot of bugs caught
Left mostly small recommendations, as well as comments for future thoughts.
multiaddrs: (peerData.addresses || []).map((address) => address.multiaddr) | ||
provs | ||
.forEach(id => { | ||
const peerData = dht.peerStore.get(id) |
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.
const peerData = dht.peerStore.get(id) | |
const peer = dht.peerStore.get(id) |
We initially were going with the peerData
naming for peerStore records, but we decided after the DHT refactor to just call peer
and make it consistent here later on.
.forEach(id => { | ||
const peerData = dht.peerStore.get(id) | ||
|
||
if (peerData) { |
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.
if (peerData) { | |
if (peer) { |
|
||
if (peerData) { | ||
out.push({ | ||
id: peerData.id, |
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.
id: peerData.id, | |
id: peer.id, |
if (peerData) { | ||
out.push({ | ||
id: peerData.id, | ||
multiaddrs: peerData.addresses |
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.
multiaddrs: peerData.addresses | |
multiaddrs: peer.addresses |
if (!peerData) { | ||
throw errcode(new Error('No peer data found in peer store'), 'ERR_NOT_FOUND') | ||
} | ||
|
||
return { | ||
id: peerData.id, |
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.
id: peerData.id, | |
id: peer.id, |
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.
There are other occurrences in this file, I will not flag all of them
if (!peerData) { | ||
throw errcode(new Error('No peer data found in peer store'), 'ERR_NOT_FOUND') | ||
} | ||
|
||
return { | ||
id: peerData.id, | ||
multiaddrs: peerData.addresses.map((address) => address.multiaddr) |
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.
multiaddrs: peerData.addresses.map((address) => address.multiaddr) | |
multiaddrs: peer.addresses.map((address) => address.multiaddr) |
Adds types to source code, tests can be done later. Updates all deps.
2ce1bf4
to
2de1694
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.
Minor left stuff
* @param {Uint8Array} key | ||
* @param {Object} [options] | ||
* @param {boolean} [options.shallow] shallow query (default: false) | ||
* @returns {AsyncIterable<{ id: PeerId, multiaddrs: Multiaddr[] }>} |
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.
@achingbrain can we have this back?
Co-authored-by: Vasco Santos <vasco.santos@moxy.studio>
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.
LGTM
Adds types to source code, tests can be done later.
Updates all deps.
Depends on: