Skip to content
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

feat!: frontend should handle peer discovery #550

Merged
merged 7 commits into from
Apr 11, 2024
Merged

Conversation

EvanHahn
Copy link
Contributor

@EvanHahn EvanHahn commented Apr 11, 2024

See also: digidem/comapeo-mobile#245.

I recommend reviewing this one commit at a time.

Our peer discovery system is unreliable on some networks. We'd like to replace it with a native local service discovery system.

To do this, the frontend needs to tell the backend about peers it finds. In other words, we need to "invert" peer discovery.

Here's pseudocode for what the frontend needs to do:

const { name, port } = await mapeoApi.startLocalPeerDiscoveryServer()
zeroconf.publishService({ name, port })
zeroconf.on('resolved', (peer) => {
  mapeoApi.connectPeer(peer)
})

This implements that by removing our mDNS system and replacing it with these public APIs.

Closes #474.

@EvanHahn EvanHahn marked this pull request as ready for review April 11, 2024 15:16
@EvanHahn EvanHahn requested a review from achou11 April 11, 2024 15:17
Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes seem good to me!

src/discovery/local-discovery.js Outdated Show resolved Hide resolved
@@ -25,10 +25,10 @@ export const ERR_DUPLICATE = 'Duplicate connection'
*/
export class LocalDiscovery extends TypedEmitter {
#identityKeypair
#name = randomBytes(8).toString('hex')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: do you think it will it ever be useful to provide a custom name? can consider a constructor opt for this if so

Copy link
Contributor Author

@EvanHahn EvanHahn Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be useful in future but not at present, so I'm going to skip it for now.

@EvanHahn EvanHahn merged commit 9a523e5 into main Apr 11, 2024
2 of 4 checks passed
@EvanHahn EvanHahn deleted the inverted-peer-discovery branch April 11, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve local device discovery on different networks
2 participants