Conversation
f2bc0e0 to
4fa279c
Compare
ad978f0 to
a45a814
Compare
| const dns = createDNSAPI() | ||
| const isOnline = createIsOnlineAPI({ network }) | ||
| // @ts-ignore This type check fails as options. | ||
| // libp2p can be a function, while IPNS router config expects libp2p config |
There was a problem hiding this comment.
This is not an easy fix. I recommend we do it individually
| const exportKey = (name, password, options) => | ||
| keychain.exportKey(name, password, options) | ||
| const exportKey = (name, password) => | ||
| keychain.exportKey(name, password) |
There was a problem hiding this comment.
We do not support Aborts/options in keychain.
| */ | ||
| async function subscribe (topic, handler, options) { | ||
| const { libp2p } = await network.use(options) | ||
| // @ts-ignore Libp2p Pubsub is deprecating the handler, using the EventEmitter |
There was a problem hiding this comment.
At some point we might get https://github.com/libp2p/js-libp2p/blob/0.30.x/src/pubsub-adapter.js this out. I can add in a follow up PR the handlers behaviour for IPFS
| async function ls (options) { | ||
| const { libp2p } = await network.use(options) | ||
| return libp2p.pubsub.getTopics(options) | ||
| return libp2p.pubsub.getTopics() |
There was a problem hiding this comment.
Pubsub has no options for getTopics + getSubscribers
|
|
||
| const { peerId, keychain, isNew } = await loadRepo(repo, options) | ||
|
|
||
| // TODO: throw error? |
There was a problem hiding this comment.
Keychain is not available in init IIRC. That is why it might exist or not
There was a problem hiding this comment.
So there was / is this code:
js-ipfs/packages/ipfs-core/src/components/storage.js
Lines 137 to 156 in 2bafc88
And this code path
js-ipfs/packages/ipfs-core/src/components/storage.js
Lines 214 to 230 in 2bafc88
And I'm still not sure why libp2p is created in two different ways to be honest. It would be good to get these cleaned up (in some followup). I'd also happily pair on this as lack of uncertainty here really bothered me.
There was a problem hiding this comment.
I agree with you on this, it also confused me. We should improve this on a followup
40a6ac5 to
95499e7
Compare
Co-authored-by: Hugo Dias <hugomrdias@gmail.com>
2fc5b0c to
c487184
Compare
c487184 to
2bafc88
Compare
Gozala
left a comment
There was a problem hiding this comment.
Thanks for the pull @vasco-santos, looks good to me.
|
|
||
| const { peerId, keychain, isNew } = await loadRepo(repo, options) | ||
|
|
||
| // TODO: throw error? |
There was a problem hiding this comment.
So there was / is this code:
js-ipfs/packages/ipfs-core/src/components/storage.js
Lines 137 to 156 in 2bafc88
And this code path
js-ipfs/packages/ipfs-core/src/components/storage.js
Lines 214 to 230 in 2bafc88
And I'm still not sure why libp2p is created in two different ways to be honest. It would be good to get these cleaned up (in some followup). I'd also happily pair on this as lack of uncertainty here really bothered me.
|
|
||
| const { peerId, keychain, isNew } = await loadRepo(repo, options) | ||
|
|
||
| // TODO: throw error? |
There was a problem hiding this comment.
Seems like throw is not a great option here, if we keychain does not exists we should probably create it ? Otherwise I don't see how user is supposed to use this without getting an error.
There was a problem hiding this comment.
As far as I understand, Keychain will always exist at this point as this is only used after start. The problem is that the types are not sure about it.
| async function disconnect (addr, options) { | ||
| const { libp2p } = await network.use(options) | ||
| return libp2p.hangUp(addr, options) | ||
| return libp2p.hangUp(addr) |
There was a problem hiding this comment.
Just to do the same thing you did with dial
| return libp2p.hangUp(addr) | |
| await libp2p.hangUp(addr) |
| const filters = require('libp2p-websockets/src/filters') | ||
| const transportKey = WS.prototype[Symbol.toStringTag] | ||
|
|
||
| module.exports = () => ({ |
There was a problem hiding this comment.
this could be just an object since it doesn't take any input, it would reduce LoC in the others files
There was a problem hiding this comment.
The main reason I created a function here is to enable the options to be customized from the caller without modifying the actual content thanks to the new reference. We had issues in the past in libp2p tests, where its base configuration used in tests was modified in some tests, which caused other tests to have different configurations than expected required.
We currently only use it directly and I can turn that into an object. What do you think?
There was a problem hiding this comment.
sounds good maybe freeze it to be safe if it makes sense.
Integrates
libp2p@0.30release.This PR changes needed types accordingly.
Needs:
A note should be included in the next release notes regarding the
websocketsupdate to anticipate possible problems for local environment tests of IPFS users