feat: filter IPFS addrs correctly #62
Conversation
dryajov
force-pushed
the
feat/circuit-interop
branch
from
e012fdd to
a49b546
Compare
dryajov
requested review from
dignifiedquire and
whyrusleeping
and removed request for
a team
src/index.js
Outdated
| @@ -2,7 +2,6 @@ | |||
|
|
|||
| const connect = require('pull-ws/client') | |||
| const mafmt = require('mafmt') | |||
| const includes = require('lodash.includes') | |||
There was a problem hiding this comment.
can this module be removed from the dependencies now?
src/index.js
Outdated
| return mafmt.WebSockets.matches(ma) || | ||
| mafmt.WebSocketsSecure.matches(ma) || | ||
| mafmt.WebSocketsIPFS.matches(ma) || | ||
| mafmt.WebSocketsSecureIPFS.matches(ma) |
There was a problem hiding this comment.
Following multiformats/js-mafmt#16 (comment), if you do multiple assertions anyways, why having some mix and some not. Just assert for IPFS separately.
From this Boolean assertion, seems that having IPFS is actually not relevant.
There was a problem hiding this comment.
I wasn't sure when I coded this if it was the best way of doing it. The main idea behind leaving the non IPFS version was fear of breaking something else. I think in this case it should be safe to just do:
mafmt.WebSocketsIPFS.matches(ma) ||
mafmt.WebSocketsSecureIPFS.matches(ma)
There was a problem hiding this comment.
As for being able to just do IPFS.match(ma) as you pointed out in multiformats/js-mafmt#16 (comment), I don't think that would work, since it would also match TCP and WebRTC addrs here.
dryajov
force-pushed
the
feat/circuit-interop
branch
from
08ab66b to
e600990
Compare
dryajov
changed the title
dryajov
force-pushed
the
feat/circuit-interop
branch
from
e600990 to
58410a2
Compare
dryajov
changed the title
|
Seems like travis is failing on an IO error, not sure what it means exactly - I don't see a specific error besides Also, I don't have a way of triggering rebuilds so my ability to troubleshoot this is limited. |
|
@dryajov use the latest CI configs from https://github.com/ipfs/js-ipfs |
ghost
assigned
package.json
Outdated
| "coverage-publish": "aegir-coverage publish" | ||
| "lint": "aegir lint", | ||
| "build": "aegir build", | ||
| "test": "aegir test --target node --target browser", |
There was a problem hiding this comment.
try with --no-parallel and increasing the timeout for the test with multiple writes
|
@diasdavid this is passing now in both travis and circle... had to go pretty aggressive with the timeout tho, will need to fine tune - but I think we can merge safely for now? |
|
@diasdavid I removed the global timeouts and this should be ready to merge. |
* feat: resolve 0 addresses * chore: upgrading pull-ws * chore: update circle CI * chore: update gitignore
This depends on mafmt PR - multiformats/js-mafmt#16