feat: use PubSub API directly from libp2p#1215
Conversation
ghost
assigned
daviddias
force-pushed
the
feat/libp2p-pubsub
branch
from
a2a566b to
11e8f77
Compare
daviddias
force-pushed
the
feat/libp2p-pubsub
branch
from
f339310 to
cdbee33
Compare
|
Done |
daviddias
force-pushed
the
feat/libp2p-pubsub
branch
from
f3f8534 to
fce326c
Compare
|
Had to fix some linting issues that were merged in Master (somehow escaped the check, shame on me for not triple checking.) |
|
CI is green \o/ Merging as this solves also some issues in Master that are preventing other PRs |
vmx
left a comment
vmx
left a comment
There was a problem hiding this comment.
I guess the error checking that got removed is now in libp2p (I haven't checked). The rest looks OK, I've mostly minor things.
| if (typeof ma === 'string') { | ||
| try { | ||
| ma = new Multiaddr(ma) | ||
| return Boolean(ma) |
There was a problem hiding this comment.
This idea comes from type checking discussion. It would be cool if isValid() functions would return false if invalid and return a valid object whatever got checked (in this case a Multiaddr).
That way you could also use isValid() to make sure you always get a valid Multiaddr object, no matter if it was already one or of if it was a string.
| if (err) { | ||
| return callback(err) | ||
| } | ||
| if (err) { return callback(err) } |
There was a problem hiding this comment.
Nitpick: I prefer not having unrelated formatting changes together with real code changes.
|
|
||
| const dfProc = DaemonFactory.create({ type: 'proc' }) | ||
| const IPFSFactory = require('ipfsd-ctl') | ||
| const fDaemon = IPFSFactory.create({ type: 'js' }) |
There was a problem hiding this comment.
I find fDaemon a strange, hard to read, uncommon name. What about just Deamon or jsDeamon.
There was a problem hiding this comment.
It's a Factory. I could call it factoryDaemon if that makes more sense for you
There was a problem hiding this comment.
Does it matter that's a factory? If you really want to know, I think it's apparent from IPFSFactor.create() that it is a factory.
This don't find this super important, so if you don't find it worth the change, feel free to leave it as it currently is.
| const dfProc = DaemonFactory.create({ type: 'proc' }) | ||
| const IPFSFactory = require('ipfsd-ctl') | ||
| const fDaemon = IPFSFactory.create({ type: 'js' }) | ||
| const fInProc = IPFSFactory.create({ type: 'proc' }) |
There was a problem hiding this comment.
I find fInProc a strange, hard to read, meaningless, uncommon name. What about just InProc or InProcDeamon.
There was a problem hiding this comment.
Same as #1215 (comment). I can use the full word if that works better :)
|
Oh, I just saw that it got already merged :) Well, then see my comments as FYI and fill free to do a new PR if you think it's worth it. |
2 participants
No description provided.