-
Notifications
You must be signed in to change notification settings - Fork 107
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
feat: provide default libp2p instance #127
Conversation
The original intention was to allow users to configure a libp2p node to their requirements but it's a non-trivial undertaking unless the user is deeply familiar with the libp2p stack. Instead, let's provide node and browser libp2p instances with sensible defaults that give the user the best chance of success on first try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, just a question.
"@ipld/dag-cbor": "^9.0.0", | ||
"@ipld/dag-json": "^10.0.1", | ||
"@libp2p/websockets": "^5.0.3", | ||
"@libp2p/websockets": "^6.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be "optional" dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for others that are browser only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, optional dependencies means "if this dependency failed to install, carry on anyway".
I think what you are saying is "let the user decide whether to install this dep or not", which is what "peerDependencies" used to mean, now npm installs peer deps whether you like it or not so I'm not sure there's a way to do that.
## [@helia/interface-v1.1.0](https://github.com/ipfs/helia/compare/@helia/interface-v1.0.0...@helia/interface-v1.1.0) (2023-05-19) ### Features * provide default libp2p instance ([#127](#127)) ([45c9d89](45c9d89)), closes [#121](#121) ### Trivial Changes * bump aegir from 38.1.8 to 39.0.4 ([#111](#111)) ([2156568](2156568))
🎉 This PR is included in version @helia/interface-v1.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version helia-v1.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there are any docs or examples we need to update?
], | ||
streamMuxers: [ | ||
yamux(), | ||
mplex() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we kill off mplex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe - do we have stats for how well used yamux is yet?
js-ipfs never shipped with it included so you'd lose compatibility with any existing deployments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, I was wanting to make sure we don't proliferate mplex, but as long as we prefer yamux that is the key thing (which you're doing). I'm not aware of harm of one also offering mplex, and assuming there are no downsides to it, I'm game to include it for compatibility.
That said, if go-libp2p ships without mplex by default, I think we'd be fine to remove it. I don't have stats here and I don't think we need to block to find that out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Muxers are negotiated in-order so yamux will be preferred over mplex by any peer that supports both.
], | ||
streamMuxers: [ | ||
yamux(), | ||
mplex() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we remove?
list: [ | ||
'/dnsaddr/bootstrap.libp2p.io/p2p/QmNnooDu7bfjPFoTZYxMNLWUQJyrVwtbZg5gBMjTezGAJN', | ||
'/dnsaddr/bootstrap.libp2p.io/p2p/QmQCU2EcMqAqQPR2i9bChDtGNJchTbq5TbXJJ16u19uLTa', | ||
'/dnsaddr/bootstrap.libp2p.io/p2p/QmbLHAnMoJPWSCR5Zhtx6BHJX9KiKNN6tpvbUcqanj75Nb', | ||
'/dnsaddr/bootstrap.libp2p.io/p2p/QmcZf59bWwK5XFi76CZX8cbJ4BhTzzA3gU1ZjYZcYW3dwt' | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicated. Maybe factor it out, and also add comments on where this list comes from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -28,11 +27,11 @@ export type { Await, AwaitIterable } from 'interface-store' | |||
/** | |||
* The API presented by a Helia node. | |||
*/ | |||
export interface Helia { | |||
export interface Helia<T = Libp2p> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have to spend time on this, but just for my own learning, I'm curious why we had to parameterize Helia.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Libp2p is parameterised by the service map, so the generic is added here to let us reflect that in the instance of Helia returned from the factory.
List of bootstrap nodes is in one place now and links to the source. Refs: #127 (comment)
## [helia-v1.1.2](helia-v1.1.1...helia-v1.1.2) (2023-05-19) ### Bug Fixes * dedupe bootstrap list ([#129](#129)) ([bb5d1e9](bb5d1e9)), closes [/github.com//pull/127#discussion_r1199152374](https://github.com/ipfs//github.com/ipfs/helia/pull/127/issues/discussion_r1199152374) ### Dependencies * update sibling dependencies ([d33c843](d33c843))
The original intention was to allow users to configure a libp2p node to their requirements but it's a non-trivial undertaking unless the user is deeply familiar with the libp2p stack.
Instead, let's provide node and browser libp2p instances with sensible defaults that give the user the best chance of success on first try.
Fixes #121