libp2p / js-libp2p-interfaces

Contains test suites and interfaces you can use to implement the various components of js libp2p.
Other
75 stars 27 forks source link

[transport] Document filter function #31

Closed alanshaw closed 4 years ago

alanshaw commented 6 years ago

filter filters the list of multiaddrs that this transport is willing to dial to.

It's a required function but is not documented.

gitGksgk commented 6 years ago

Hello, has this been fixed? Using js-ipfs won't automatically produce this issue unless npm install wrtc which means using wrtc for webrtc support. AFAIK without this js-ipfs's peer discovery seems not working by default. However, after installing I got a t.filter is not a function as is known. I am using js-ipfs 0.31.7, but in latest release corresponding code in src\http\index.js seemingly unchanged, this error might persists. here's the corresponding code :

        if (wrtc || electronWebRTC) {
          const using = wrtc ? 'wrtc' : 'electron-webrtc'
          console.log(`Using ${using} for webrtc support`)
          const wstar = new WStar({ wrtc: (wrtc || electronWebRTC) })
          libp2p.modules.transport = [TCP, WS, wstar]
          libp2p.modules.peerDiscovery = [MulticastDNS, Bootstrap, wstar.discovery]
        }

the issue, as alanshaw has pointed out, is because Transport filter not implemented. tried 0.32.3, the error persists. step to reproduce:

  1. get js-ipfs, npm install
  2. same directory, npm install wrtc
  3. node src\cli\bin.js daemon it will show:
Initializing daemon...
Using wrtc for webrtc support
J:\js-ipfs-update\js-ipfs\node_modules\libp2p\src\index.js:145
      if (t.filter(multiaddrs).length > 0) {
            ^

TypeError: t.filter is not a function
    at _modules.transport.forEach (J:\js-ipfs-update\js-ipfs\node_modules\libp2p\src\index.js:145:13)
    at Array.forEach (<anonymous>)
    at Node.start (J:\js-ipfs-update\js-ipfs\node_modules\libp2p\src\index.js:136:29)
    at gotConfig (J:\js-ipfs-update\js-ipfs\src\core\components\libp2p.js:95:26)
    at store.get (J:\js-ipfs-update\js-ipfs\node_modules\ipfs-repo\src\config.js:45:9)
    at fs.readFile (J:\js-ipfs-update\js-ipfs\node_modules\datastore-fs\src\index.js:218:7)
    at J:\js-ipfs-update\js-ipfs\node_modules\graceful-fs\graceful-fs.js:78:16
    at FSReqWrap.readFileAfterClose [as oncomplete] (fs.js:511:3)

J:\js-ipfs-update\js-ipfs>node src\cli\bin.js version
js-ipfs version: 0.32.3
gitGksgk commented 6 years ago

emmm.... I've got it wrong. t.filter is implemented. This issue is because of inconsistent arguement type in transport

const WStar = require('libp2p-webrtc-star')
const TCP = require('libp2p-tcp')
const WS = require('libp2p-websockets')

   if (wrtc || electronWebRTC) {
          const using = wrtc ? 'wrtc' : 'electron-webrtc'
          console.log(`Using ${using} for webrtc support`)
          const wstar = new WStar({ wrtc: (wrtc || electronWebRTC) })
          libp2p.modules.transport = [TCP, WS, wstar]
          libp2p.modules.peerDiscovery = [MulticastDNS, Bootstrap, wstar.discovery]
        }

in libp2p.modules.transport = [TCP, WS, wstar] TCP or WS is a class while wstar is an instance of a class....
changing it to libp2p.modules.transport = [TCP, WS, WStar] resolves the issue, though it may lead to some unexpected behavior... same thing goes with libp2p.modules.peerDiscovery. but I have no idea on how to formalize wstar.discovery

jacobheun commented 6 years ago

On startup libp2p should check if the transport is an instance of a class/function and handle it appropriately.

In addition to documenting .filter tests need to be added so that the transport modules are being adequately checked.

pynchmeister commented 5 years ago

Am I correct to assume that this is still being fixed?

jacobheun commented 5 years ago

It still needs to be documented and added, yes, but the core transports do have the appropriate support. This would be a nice thing to roll out with the async/await updates to transports. PRs are welcome and appreciated!

jacobheun commented 4 years ago

Fixed during the migration to async await, https://github.com/libp2p/js-interfaces/tree/master/src/transport#filtering-addresses.