feat: peer-discovery not using peer-info

[vasco-santos]

In the context of deprecating peer-info as described on libp2p/js-libp2p#589, this PR changes the peer-discovery interface to emit an object containing an id property and a multiaddrs property.

Future discovery services might gather other data, such as known protocols. However, we should specify those optional parameters when the time comes.

BREAKING CHANGE: peer-discovery emits object with id and multiaddrs properties

[vasco-santos]

We currently do not have meaningful tests for this interface yet. Should we try to leverage this change to do it?

If we specify that the events should be emitted via a function _onPeer(peerId, multiaddrs), we could call this function. I am not a big fan of this solution though. We could also have a base class that discovery services extend and it would do the emit for them.

cc @jacobheun

[jacobheun]

We currently do not have meaningful tests for this interface yet. Should we try to leverage this change to do it?

Yes we should.

If we specify that the events should be emitted via a function _onPeer(peerId, multiaddrs), we could call this function. I am not a big fan of this solution though. We could also have a base class that discovery services extend and it would do the emit for them.

What are you trying to get out of this? It seems like it's doing some unnecessary restrictions. If we specify the API requirements and add tests that should be sufficient.

[vasco-santos]

What are you trying to get out of this? It seems like it's doing some unnecessary restrictions. If we specify the API requirements and add tests that should be sufficient.

I agree with not being nice to add unnecessary restrictions. However, we need to have a way of simulating the peer event from the discovery service and verify that it provided the peerId and multiaddrs. Any idea on how we should trigger it on a generic way?

[jacobheun]

Any idea on how we should trigger it on a generic way?

I think this should just be handled by the common setup requirements of the test. Such as, once start is called, the handler discovery.on('peer', handler) should be called repeatedly until stop is called. This would allow us to validate start, stop and whatever required event handler methods we add.

The interface tests shouldn't trigger anything, just call methods.

[jacobheun]

Found a few things. Also, I think we should merge this into a new minor branch since we'll need to update a few of the other interfaces for the peer-info change.

[jacobheun]

Should we change peerInfo to peerData everywhere to avoid confusion?

[vasco-santos]

yes please 😂

[jacobheun]

This shouldn't need to be here.

Suggested change

await new Promise(resolve => setTimeout(resolve, 10))

[vasco-santos]

Found a few things. Also, I think we should merge this into a new minor branch since we'll need to update a few of the other interfaces for the peer-info change.

What about an approach with a pre-release, so that we can start merging all the PRs related to peer-discovery as release candidates, and then we only need to change for the final release? All the needed PRs are open now as well, so I think the sooner we get them, the sooner we unblock other PRs

[jacobheun]

What about an approach with a pre-release, so that we can start merging all the PRs related to peer-discovery as release candidates, and then we only need to change for the final release?

These aren't mutually exclusive options. We can do pre releases / RCs from any branch. The main reason for the separate branch is to keep master open for any patches that might be needed before this is released.

We can always cherry pick patches onto the latest tag and release that, but we'd also have to remember to pull those patches, as needed back into master. Putting everything into a new branch avoids the need to remember because we'll get the change when we go to merge into master.

[jacobheun]

This also replaces #19 right?

[vasco-santos]

These aren't mutually exclusive options. We can do pre releases / RCs from any branch. The main reason for the separate branch is to keep master open for any patches that might be needed before this is released.

We can always cherry pick patches onto the latest tag and release that, but we'd also have to remember to pull those patches, as needed back into master. Putting everything into a new branch avoids the need to remember because we'll get the change when we go to merge into master.

Yes, let's do it in a new branch then

This also replaces #19 right?

Oh yes! Just closed it, completely forgot about that initial PR

[jacobheun]

LGTM, updated the base branch to 0.3.x