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

WebRTC connectivity guide #377

Merged
merged 25 commits into from
Jun 12, 2024
Merged

WebRTC connectivity guide #377

merged 25 commits into from
Jun 12, 2024

Conversation

2color
Copy link
Contributor

@2color 2color commented May 14, 2024

This PR adds a new guide on WebRTC with js-libp2p for browser-to-browser connectivity.

TODO

Tasks

Preview Give feedback

@2color 2color marked this pull request as ready for review May 24, 2024 14:59
@2color 2color requested a review from p-shahi May 28, 2024 13:41
@2color 2color closed this May 29, 2024
@2color 2color reopened this May 29, 2024
@2color 2color closed this May 29, 2024
@2color 2color reopened this May 29, 2024
Copy link

vercel bot commented May 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
libp2p-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 11, 2024 5:42pm

@2color
Copy link
Contributor Author

2color commented May 31, 2024

I just made Vercel previews public, so anyone opening this PR should be able to preview

Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

This is great. Some thoughts:

  • Nothing here is browser specific, all of this applies to using the WebRTC transport to dial browsers from Node.js so it might be worth mentioning that?
    • Dialling browsers from non-browsers is only possible in js-libp2p right now, that's not really touched on here
  • A lot of time is spent discussing how to automatically discover the listener peer's address, including using GossibSub to do so
    • It's rare to want to dial a specific peer for no reason, normally you want to dial a peer because they have something you want like an IPFS block, in which case you would have found their address during a search for providers
    • GossipSub is a big dep that causes the node to do a lot of work, it's inclusion comes with a lot of overhead
  • Language/diagrams could be simplified by using roles (e.g. Dialer/Listener) instead of Peer 1/Peer 2
  • The role of the relay/bootstrapper becomes a bit overloaded since it's used for connection establishment but also peer discovery
    • Maybe it could be two separate nodes to make it clear that the peer discovery part isn't a hard dependency of establishing the connection, it's just a way for the dialler to not have to copy/paste the multiaddr of the listener?
    • Alternatively peer discovery could be moved out of scope here and covered elsewhere?
    • The name "bootstrapper" is a bit confusing since what we normally refer to as "bootstrappers" are nodes run to enable populating routing tables
  • The context switch between js and go for the relay is a bit jarring, also considering people reading this will like be JS people, but it is covered at the end of the article.
    • It's a little odd that we recommend using go-libp2p for the relay because it has WebTransport, but WebTransport is still pretty broken in browsers. WebSockets, although tedious to configure, do work very well and are available everywhere. Not sure there's a good answer here.

content/guides/getting-started/browser-to-browser.md Outdated Show resolved Hide resolved
content/guides/getting-started/browser-to-browser.md Outdated Show resolved Hide resolved
content/guides/getting-started/browser-to-browser.md Outdated Show resolved Hide resolved
content/guides/getting-started/browser-to-browser.md Outdated Show resolved Hide resolved
content/guides/getting-started/browser-to-browser.md Outdated Show resolved Hide resolved
content/guides/getting-started/browser-to-browser.md Outdated Show resolved Hide resolved
content/guides/getting-started/browser-to-browser.md Outdated Show resolved Hide resolved
content/guides/getting-started/browser-to-browser.md Outdated Show resolved Hide resolved
content/guides/getting-started/browser-to-browser.md Outdated Show resolved Hide resolved
content/guides/getting-started/browser-to-browser.md Outdated Show resolved Hide resolved
@2color
Copy link
Contributor Author

2color commented Jun 4, 2024

  • Nothing here is browser specific, all of this applies to using the WebRTC transport to dial browsers from Node.js so it might be worth mentioning that?

    • Dialling browsers from non-browsers is only possible in js-libp2p right now, that's not really touched on here

This is pretty cool. I never thought about it this way, but will definitely weave this into the guide.

  • The name "bootstrapper" is a bit confusing since what we normally refer to as "bootstrappers" are nodes run to enable populating routing tables

Do you think Relay is a better term? Which term would you recommend here?

The context switch between js and go for the relay is a bit jarring, also considering people reading this will like be JS people, but it is covered at the end of the article.

  • It's a little odd that we recommend using go-libp2p for the relay because it has WebTransport, but WebTransport is still pretty broken in browsers. WebSockets, although tedious to configure, do work very well and are available everywhere. Not sure there's a good answer here.

Fair point. I'll give writing the relay in node.js with WebSockets. Since the guide is only running in localhost, we don't need a certificate, so this may work well.

@2color
Copy link
Contributor Author

2color commented Jun 10, 2024

@achingbrain Mind giving this another review. I've addressed all your feedback and think this is ready to be merged.

@2color 2color changed the title Add browser-to-browser WebRTC connectivity guide WebRTC connectivity guide Jun 11, 2024
Great job!!

I cleaned up passive voice as best as I could.
Copy link
Contributor

@dhuseby dhuseby left a comment

Choose a reason for hiding this comment

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

This is great!! I just cleaned up some passive voice for clarity.

@dhuseby
Copy link
Contributor

dhuseby commented Jun 11, 2024

LGTM. Send it.

@2color 2color merged commit 5886d88 into master Jun 12, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants