Skip to content
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

new config #166

Merged
merged 5 commits into from
Jun 28, 2018
Merged

new config #166

merged 5 commits into from
Jun 28, 2018

Conversation

daviddias
Copy link
Member

@daviddias daviddias commented Feb 20, 2018

For #46. Takes into account:

Would love to get a ton of feedback from libp2p users.


@ghost ghost assigned daviddias Feb 20, 2018
@ghost ghost added the status/in-progress In progress label Feb 20, 2018
This was referenced Feb 20, 2018
README.md Outdated
}

super(modules, peerInfo, peerBook, options)
// overload any defaults of your bundle
Object.assign(options, _options)
Copy link
Member

@alanshaw alanshaw Feb 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double checking you want an assign? You've gone to a lot of trouble to establish defaults which are easily removed. For example, if I pass an options object like { config: { addrs: { listen: ['/ip4/0.0.0.0/tcp/5555'] } } } I blow away all the other defaults that have been set for peerDiscovery, protocolMuxing etc.

Do you need a merge or a smart assign instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, it need to be a deepMerge.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

look into lodash defaults/deepDefaults instead, they do what you need.

README.md Outdated
// DHT is passed as its own enabling PeerRouting, ContentRouting and DHT itself components
dht: DHT
config: { // The config object is the part of the config that can go into a file, config.json.
addrs: { // Multiaddrs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The peerInfo already contains multiaddrs. What happens with that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They get added/merged. This is to enable the config to be a single JSON file that has the addrs without any complex setup.

README.md Outdated
peerInfo: _peerInfo // The Identity of your Peer
peerBook: _peerBook // Where peers get tracked, if undefined libp2p will create one instance
modules: {
transport: [
Copy link
Member

@mkg20001 mkg20001 Feb 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some kind of first and second class transport system is needed.
For example: the new ws-star needs the libp2p swarm to dial the server via WS but WS is independent from the swarm.
(Edit: That means that WS would be a first class transport and ws-star a second class)
So any listen/dial stuff for first class transports would be setup before the second class transports are setup (so dialing does not fail)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both transports will be first class with #130 occurring during the #159. Not the concern of this proposal/PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we had a proposal for transport priorities at some point, but it was rejected.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dryajov link it please

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

I'm confused, wrong c&p?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad, this is the correct one - libp2p/js-libp2p-tcp#78

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dryajov that was a proposal for a pseudo priority based on what the module says. It needs to be a user defined.

README.md Outdated
transport: {
secio: {
spdy: {
'*': '*'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this exactly mean? Is the thing on the left the multiprotocol for secio and the thing on the right the protocol that is actually being dialed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good questions.

  1. The nested'ness here is to represent order/options. This says "After the transport, always do SECIO, after SECIO you can do SPDY or MPLEX, then do whatever"

  2. re: '*': '*' - My intention is to mean "Now protocol mux whatever protocol you want".

README.md Outdated
ann: [] // To announce/share with the network (i.e DNS addr)
notAnn: [] // To keep private
},
peerDiscovery: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How will the options be passed to the module here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They will, will present these details after this first pass on the looks of the config.

README.md Outdated
interval: 1000 // ms
enabled: true
},
webrtc-star: { // webrtc-star options
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not consistent, should be webrtcStar

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch 👍

README.md Outdated
config: { // The config object is the part of the config that can go into a file, config.json.
addrs: { // Multiaddrs
listen: [] // To listen
ann: [] // To announce/share with the network (i.e DNS addr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of these abbraviations, makes it difficult to understand what it really is unless you have context.

Also, taking inspiration from go-ipfs, it would turn out something like this:

{
  addresses: {
    swarm: [] // to listen
    announce: [] // for sure to announce
    noAnnounce: [] // stop these from announcing at all cost
    api: ''
    gateway: ''
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

@daviddias
Copy link
Member Author

I've continue my work on this PR. Made some progress and overall I like where this is headed (which is eventually enabling simpler configuration and #130).

I tried passing the responsibility completely to libp2p to instantiate all of its modules (which was not the case for Transports and Discovery services) but I hit the 🐓&🥚 of transports such as WebRTC that might require special binding modules such as wrtc.

I do want to make sure that everything that goes on config is serializable to JSON and I do want libp2p to be able to control the instantiation of modules, however, I believe I can not escape the need to be able to pass already instantiated modules too. It is not hard, just annoying because now I have to add a hint to the module to tells me if the reference is to an instance or to a class.

If this just sounded like alien 👽 mumbo jumbo to you, do not worry, I'll carry on with the coding and once it is fully ready, it will provide more clarity. If you are interested in understanding, let me know :)

@dryajov
Copy link
Member

dryajov commented Mar 20, 2018

I have to add a hint to the module to tells me if the reference is to an instance or to a class.

This sounds like you're recreating IoC (inversion of control) containers all over :) There are some out there that do that already.

@daviddias daviddias added the P0 Critical: Tackled by core team ASAP label Jun 4, 2018
@daviddias daviddias changed the title new config proposal new config Jun 4, 2018
README.md Outdated
EXPERIMENTAL: { // Experimental features ("behind a flag")
pubsub: true,
dht: true
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make EXPERIMENTAL part of the config.

README.md Outdated
mplex: '*'
}
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feature will have to be implemented at the Switch Level

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before merging, I'll remove these options from the README and continue working on this feature design at #201

README.md Outdated
@@ -101,47 +101,90 @@ libp2p becomes very simple and basically acts as a glue for every module that co
```JavaScript
// Creating a bundle that adds:
// transport: websockets + tcp
// stream-muxing: SPDY
// stream-muxing: spdy & mplex
// crypto-channel: secio
// discovery: multicast-dns

const libp2p = require('libp2p')
const TCP = require('libp2p-tcp')
const WS = require('libp2p-websockets')
const spdy = require('libp2p-spdy')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uppercase

README.md Outdated
// crypto-channel: secio
// discovery: multicast-dns

const libp2p = require('libp2p')
const TCP = require('libp2p-tcp')
const WS = require('libp2p-websockets')
const spdy = require('libp2p-spdy')
const mplex = require('libp2p-mplex')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uppercase + needs to be included in the modules

README.md Outdated
// crypto-channel: secio
// discovery: multicast-dns

const libp2p = require('libp2p')
const TCP = require('libp2p-tcp')
const WS = require('libp2p-websockets')
const spdy = require('libp2p-spdy')
const mplex = require('libp2p-mplex')
const secio = require('libp2p-secio')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uppercase

src/index.js Outdated
setImmediate(() => discovery.stop(() => {}))
if (this._modules.peerDiscovery) {
this._discovery.forEach((d) => {
setImmediate(() => d.stop(() => {}))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something for another PR, but this smothers any error while stopping and means that when callback is called for stop all the discoveries might not have stopped yet.

src/index.js Outdated
@@ -269,25 +296,26 @@ class Node extends EventEmitter {
this._getPeerInfo(peer, (err, peerInfo) => {
if (err) { return callback(err) }

this.switch.hangUp(peerInfo, callback)
this._switch.hangUp(peerInfo, callback)
})
}

ping (peer, callback) {
assert(this.isStarted(), NOT_STARTED_ERROR_MESSAGE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again for another PR - we need to call callback with this error or the process will quit.

@alanshaw
Copy link
Member

Can we merge and release this as is and finish the examples in separate PRs?

@jacobheun
Copy link
Contributor

@alanshaw I am working on this today. I think we need to resolve, #166 (comment) first, which I will work on. With the changes to the config, I feel it's important to make sure we have proper validation there so users aren't confused why their config is not working.

I should have this resolved today or tomorrow.

@alanshaw
Copy link
Member

🚀 awesome @jacobheun! Apologies I hadn't clocked #166 (comment)

docs: update echo example
docs: update libp2p in browser example
docs: update pubsub example
docs: update peer and content routing examples
docs: update discovery mechanisms example
docs: update encrypted comms example
docs: update protocol and stream muxing example
fixed examples and tests as a result of the config validation
test: add config tests and fix bugs
fix: linting
@jacobheun
Copy link
Contributor

jacobheun commented Jun 27, 2018

Okay, so I think I have everything checked off. I've rebased from master so everything is up to date (@diasdavid fyi on the rebase in case you pull).

I've coupled the latest changes into two commits, hopefully this will help keep things clear.

  1. The first commit is the updated examples. I ran through all of them a final time after my last changes and everything runs. Woo!
  2. I added Joi to validate the configuration. The second commit is pretty much about that, but also includes some of the misc changes @alanshaw mentioned to add to future PRs. This resulted in some code clean up, as we're able to rely on thrown errors from config validation to avoid some unnecessary property checks. It also exposed some issues with the tests not using proper configs, so those are fixed too.

I added tests for the new config validation. There is some custom logic there for peerDiscovery in modules and config, as well as experimental dht.

Pubsub: pubsub should now only be initialized if it's enabled via EXPERIMENTAL. Calling any methods on pubsub when it's not enabled will result in an error.

Provided there are no changes requested from those 2 latest commits, I think this should be good to go.

@@ -1,2 +1,2 @@
// Warning: This file is automatically synced from https://github.com/ipfs/ci-sync so if you want to change it, please change it there and ask someone to sync all repositories.
javascript()
javascript(['nodejs_versions': ['8.11.3']])
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacobheun @alanshaw switched to Node.js 8 for now until wrtc has builds for node 10

1. Run the listener in window 1, `node listener.js`
2. Run the dialer in window 2, `node dialer.js`
3. Type a message in either window and hit _enter_
4. Tell youself secrets to your hearts content!
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for writing the tutorial. Helps for #186

Copy link
Member Author

@daviddias daviddias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thank you for taking this one to the finish line with flying colors, @jacobheun ❤️

@daviddias
Copy link
Member Author

daviddias commented Jun 28, 2018

Only test that failed is Jenkins related (classic Mac OS failure) //cc @victorbjelkholm

image

@daviddias daviddias merged commit 6905f1b into master Jun 28, 2018
@ghost ghost removed the status/in-progress In progress label Jun 28, 2018
@daviddias daviddias deleted the better-opts branch June 28, 2018 08:06
@vasco-santos vasco-santos mentioned this pull request Jun 28, 2018
maschad pushed a commit to maschad/js-libp2p that referenced this pull request Jun 21, 2023
maschad pushed a commit to maschad/js-libp2p that referenced this pull request Jun 21, 2023
maschad pushed a commit to maschad/js-libp2p that referenced this pull request Jun 21, 2023
maschad pushed a commit to maschad/js-libp2p that referenced this pull request Jun 21, 2023
## [2.0.2](libp2p/js-libp2p-webrtc@v2.0.1...v2.0.2) (2023-05-15)

### Bug Fixes

* use transport manager getListeners to get listen addresses ([libp2p#166](libp2p/js-libp2p-webrtc#166)) ([2e144f9](libp2p/js-libp2p-webrtc@2e144f9))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 Critical: Tackled by core team ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants