fix: emit peer:connect after all

[twoeths]

Motivation

In lodestar, when we handle "peer:connect" event, we dial the peer which gives another "peer:connect" event and it causes other issues

Motivation

In onConnect function, "peer:connect" event should be emitted after we add connection to the connections map so that when app dial the peer in "peer:connect" event handler, it uses the same/existing connection

[wemeetagain]

LGTM

[achingbrain]

This seems fine to me, and a better way to do it since libp2p should make sure it's internal tracking of connections/addresses/etc is done before informing the outside world a new peer has connected to us.

I'm a little confused though:

In lodestar, when we handle "peer:connect" event, we dial the peer

peer:connect occurs when a new connection to a peer has been created, e.g. either when we dial them or when they dial us - there's no need to dial them again in another 'peer:connect' handler since we already have a connection.

Would you not just open a protocol stream to the peer instead?

That aside, CI appears to be failing due to this change - could you please try running the tests locally and ensure they pass?

[twoeths]

peer:connect occurs when a new connection to a peer has been created, e.g. either when we dial them or when they dial us - there's no need to dial them again in another 'peer:connect' handler since we already have a connection.

Would you not just open a protocol stream to the peer instead?

what we did is to issue a p2p request in lodestar, which does a dialProtocol() behind the scene in libp2p. Since the connection is not added, it'd try to open a new one, all of this logic happen in libp2p

That aside, CI appears to be failing due to this change - could you please try running the tests locally and ensure they pass?

thanks @achingbrain , this is fixed

[mpetrunic]

Rebase, fix conflicts, add test and lgtm :)

[achingbrain]

LGTM, just needs some small improvements to the tests

[twoeths]

thanks @achingbrain, this pEvent is super helpful! Could you help with the failed CI check? It's not related to the fix so I have no ideas.

[achingbrain]

The error message is in the CI output:

[08:36:25] dependency-check (production only) [failed]
Command failed with exit code 1: dependency-check package.json src/**/*.js src/**/*.cjs dist/src/**/*.js utils/**/*.js utils/**/*.cjs --missing --no-dev -i @types/* -i @types/* --extensions cjs:detective-cjs --extensions js:detective-es6
Fail! Dependencies not listed in package.json: @libp2p/tracked-map, it-pair

Basically aegir has got better at spotting unused/missing deps. @libp2p/tracked-map and it-pair are production deps that are erroneously declared as development deps, they just need moving from the "devDependencies" block to the the "dependencies" block in the project's package.json file.

[achingbrain]

Could you rebase/merge master into this branch? It should resolve the last few problems with the build.

[achingbrain]

The fix for the final failing build job was merged as part of #1194 - this is good to go.