Skip to content
This repository was archived by the owner on Feb 12, 2024. It is now read-only.

fix(docs): update webrtc instructions for node in faq #3183

Merged
merged 2 commits into from
Jul 21, 2020
Merged

fix(docs): update webrtc instructions for node in faq #3183

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
98863ec
docs: update webrtc instructions for node
achingbrain Jul 20, 2020
5e751ff
chore: update config to include peer discovery via webrtc
achingbrain Jul 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions packages/ipfs/docs/FAQ.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,10 @@ To add WebRTC support in a IPFS node instance, do:

```JavaScript
const wrtc = require('wrtc') // or require('electron-webrtc')()
const WStar = require('libp2p-webrtc-star')
const wstar = new WStar({ wrtc })
const WebRTCStar = require('libp2p-webrtc-star')

const node = await IPFS.create({
repo: 'your-repo-path',
// start: false,
config: {
Addresses: {
Swarm: [
Expand All @@ -88,8 +86,19 @@ const node = await IPFS.create({
},
libp2p: {
modules: {
transport: [wstar],
peerDiscovery: [wstar.discovery]
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to remove discovery?

Copy link
Member Author

Choose a reason for hiding this comment

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

From the old instructions you have to new up a WebRTCStar instance to get the discovery property, but now that throws an error about providing an upgrader.

Could you make a suggested change to correct the config here please?

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/libp2p/js-libp2p/blob/v0.27.8/doc/CONFIGURATION.md#setup-webrtc-transport-and-discovery

 peerDiscovery: {
   [WebRTCStar.tag]: {
     enabled: true
   }
}

FYI I also did #3092 a while ago that uses the tag's instead of the string to avoid errors like the case sensitive that you mentioned earlier, but this is only available for discovery.

Copy link
Member

Choose a reason for hiding this comment

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

also PRd libp2p to use the tag in this missing doc: libp2p/js-libp2p#713

Copy link
Member Author

Choose a reason for hiding this comment

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

Is #3092 ready for review/merging? It's been draft for ages.

transport: [WebRTCStar]
},
config: {
peerDiscovery: {
[WebRTCStar.tag]: {
enabled: true
}
},
transport: {
WebRTCStar: {
Copy link
Member Author

@achingbrain achingbrain Jul 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It'd be really nice if libp2p could make these keys case-insensitive or throw when an unknown key is encountered - webRTCStar vs WebRTCStar is easy to get wrong.

Copy link
Member

Choose a reason for hiding this comment

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

We will work on improving the config per libp2p/js-libp2p#576. It will probably change a lot the way we config libp2p. But referencing so that we have this in attention

Copy link
Member

Choose a reason for hiding this comment

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

I just noticed in our config docs that we have this:

const transportKey = WebRTCStar.prototype[Symbol.toStringTag]

Should we use it here to avoid confusions on where the naming comes from?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I'm not sure if that makes it any clearer. It would be vastly preferable to be able to just use [WebRTCStar.tag] the same as the peer discovery config key.

Copy link
Member

Choose a reason for hiding this comment

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

We should rething that in the config issue for libp2p. From what I know, the tag was added for the discovery side of things. The transports do not include it, except for webrtc-star, as it is both transport and discovery.
Anyway, we will look into it in a future libp2p release

wrtc
}
}
}
}
})
Expand Down