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

Using an Upgrader with libp2p-webrtc-star #650

Closed
daviddahl opened this issue May 28, 2020 · 9 comments
Closed

Using an Upgrader with libp2p-webrtc-star #650

daviddahl opened this issue May 28, 2020 · 9 comments

Comments

@daviddahl
Copy link

daviddahl commented May 28, 2020

A per the discussion here: libp2p/js-libp2p-webrtc-star#218 (comment)

@vasco-santos asked me to file this issue to begin a discussion of how the Upgrader and wrtc should be / can be configured in the latest libp2p while running an application in node,js.

Type: Bug

Severity: High

Description:

Adding a libp2p-webrtc-star instance into a Transports module configuration requires an Upgrader (instance? class?) - my clumsy attempt in this fork of IPFS examples: daviddahl/js-ipfs#1 best explains what I am attempting to do with libp2p on node.js

Steps to reproduce the error:

Clone the branch here: daviddahl/js-ipfs#1
cd js-ipfs/examples/custom-libp2p && npm install && node index.js

You should see:

> node index.js 
TypeError: Cannot read property 'Symbol(Symbol.toStringTag)' of undefined
    at /home/ddahl/projects/test/js-ipfs/node_modules/libp2p/src/index.js:118:38
    at Array.forEach (<anonymous>)
    at new Libp2p (/home/ddahl/projects/test/js-ipfs/node_modules/libp2p/src/index.js:117:29)
    at /home/ddahl/projects/test/js-ipfs/examples/custom-libp2p/index.js:61:17
    at processTicksAndRejections (internal/process/task_queues.js:89:5)

If you comment out this line: https://github.com/daviddahl/js-ipfs/pull/1/files#diff-3160a5b56836421c3b48dcebbf951bd5R67 for libp2p to run but not consume the webrtc-star instance.

I just noticed an rc release for 0.28, does it impact this functionality?

@jacobheun
Copy link
Contributor

Are you actually trying to customize the upgrader, or just pass wrtc to the webrtc-star constructor?

We don't support customizing the upgrader, but it looks like you just need to customize webrtc-star.

You can pass in options to the transport via the libp2p configuration, there is an example in the config readme for webrtc-star, https://github.com/libp2p/js-libp2p/blob/v0.27.8/doc/CONFIGURATION.md#customizing-transports. This will automatically handle passing the existing upgrader into the transport.

I just noticed an rc release for 0.28, does it impact this functionality?

It won't. It does deprecate PeerInfo, so the constructor will slightly change, but this doesn't effect the functionality for forwarding transport config options.

@vasco-santos
Copy link
Member

I think that we should improve the configurability of it per transport (maybe part of #576) and we should support a way of extending the upgrader current behaviour.

In this case, we want to add config options on it. We should just get a easy way of doing it for now, and then use #576 to provide full support

@daviddahl
Copy link
Author

Are you actually trying to customize the upgrader, or just pass wrtc to the webrtc-star constructor?

Thank you for this information!
I was not trying to customize it - I was just trying to configure webrtc-star, as the exception being thrown mentioned passing in (or I assumed that) an Upgrader object.

You can pass in options to the transport via the libp2p configuration, there is an example in the config readme for webrtc-star, https://github.com/libp2p/js-libp2p/blob/v0.27.8/doc/CONFIGURATION.md#customizing-transports. This will automatically handle passing the existing upgrader into the transport.

I was on 0.27.7 as well, maybe that was part of it?

It won't. It does deprecate PeerInfo, so the constructor will slightly change, but this doesn't effect the functionality for forwarding transport config options.

^^ good to know!

I'll try again following this documentation.

@daviddahl
Copy link
Author

Update: I deleted my last comment - I do think things are working now. cheers,

@autonome
Copy link

Can this be closed now, or is there a bug still?

@daviddahl
Copy link
Author

It can be closed - everything working on my end now.

@autonome
Copy link

🎉

@autonome
Copy link

@vasco-santos is there a documentation change or update from this, or is the solution to fix #576?

@vasco-santos
Copy link
Member

We already have the docs. The problem here is more the not friendly and confusing config that we have. #576 work aims to bring a better way of configuring libp2p.

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

No branches or pull requests

4 participants