-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: peer-discovery not using peer-info #5
Conversation
f48e849
to
26d4cb4
Compare
BREAKING CHANGE: peer event emits an object with id and multiaddr instead of a peer-info
26d4cb4
to
e89629e
Compare
@@ -54,6 +56,7 @@ class PubsubPeerDiscovery extends Emittery { | |||
} | |||
|
|||
this.removeListener = this.off.bind(this) | |||
this.removeAllListeners = this.clearListeners.bind(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.
What's using this? If this is being used we likely need to get more explicit about what event api peer-discovery modules should be exposing.
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 is being used fot the tests: https://github.com/libp2p/js-libp2p-interfaces/blob/v0.3.0/src/peer-discovery/tests/index.js#L25
I agree that we should be explicit about it.
src/index.js
Outdated
@@ -31,17 +30,20 @@ class PubsubPeerDiscovery extends Emittery { | |||
* @constructor | |||
* @param {Libp2p} param0.libp2p Our libp2p node | |||
* @param {number} [param0.interval = 10000] How often (ms) we should broadcast our infos | |||
* @param {Array<Multiaddr>} param0.multiaddrs Multiaddrs used by the node to listen. |
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.
How are these intended to get passed in? I would recommend not doing this and instead we should just leverage the announce addrs. I don't believe that's done yet, so in the interim we may want to just do this.libp2p.transportManager.getAddrs()
. This removes any need to add special options for the addresses, which is not friendly for new users.
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 use the transportManager.getAddrs()
and I will open an issue to use the addressManager
in the future
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.
Removed the param, using the transport manager for now and issue created #6
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, I'll merge and create a beta tagged minor release of this.
Released a major since this was already on 1.x dist-tags:
beta: 2.0.0 latest: 1.0.0 |
In the context of deprecating
peer-info
as described on libp2p/js-libp2p#589, this PR changes thepeer-discovery
interface to emit an object containing anid
property and amultiaddrs
property.This PR also adds the new
peer-discovery
interface testsBREAKING CHANGE: peer event emits an object with id and multiaddr instead of a peer-info
Needs: