-
Notifications
You must be signed in to change notification settings - Fork 60
chore: use new content and peer routing apis #181
chore: use new content and peer routing apis #181
Conversation
d38c19e
to
bea1087
Compare
1566333
to
d2eaeee
Compare
5100466
to
9e4e657
Compare
9e4e657
to
185f9c5
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.
Just found 1 thing, otherwise this looks good.
src/message/index.js
Outdated
id: peer.id.id, | ||
addrs: peer.multiaddrs.toArray().map((m) => m.buffer) | ||
addrs: (peer.multiaddrs || []).map((m) => m && m.buffer), |
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.
What's the reasoning for the check on m (m && m.buffer
)? If it's possible for m
to be null or undefined we should do a reduce, otherwise we'll end up with undefined in the addrs
array for that value.
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.
It was me when trying to debug a bug to short circuit it. But it is not needed anymore 😆
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 🚢
In the context of deprecating
peer-info
as described on libp2p/js-libp2p#589, this PR removes thepeer-info
from being used, as well as returned in the API, in favour of returning{ id, multiaddrs }
in the same way as ipfs does. Moreover, it uses the newtopology
interfaceBREAKING CHANGE: findProviders returns id and addrs properties instead of peer-info instance
This PR sits on top of #179 and #180, and those should be merged first and this PR rebased.
Needs: