Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

chore: update libp2p runtime config #3092

Merged
merged 5 commits into from
Jul 24, 2020
Merged

chore: update libp2p runtime config #3092

merged 5 commits into from
Jul 24, 2020

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented Jun 17, 2020 •
edited
Loading

This PR removes the websocketStar discovery configuration, since it is not used anymore.

In addition, I have changed the discovery keys to use the modules tag from the module. This has created some confusion to libp2p users, and we would like people to use the tag instead of magic keys that might also be changed in the future: libp2p/js-libp2p#664 (comment)

vasco-santos
vasco-santos commented Jun 17, 2020
View reviewed changes
packages/ipfs/src/core/runtime/libp2p-browser.js Outdated
@@ -1,5 +1,6 @@
'use strict'

const Bootstrap = require('libp2p-bootstrap')
Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed that Bootstrap is only required in components/libp2p. Is this a bundle optimization?

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @achingbrain

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK the inline requires are to speed up node start-up time.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, so I will add a comment for that and keep the Bootstrap as it was and only use the tag for the other one

Copy link
Contributor

Choose a reason for hiding this comment

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

iirc the inline requires were done to reduce the cli execution time, not startup time (ie: if it's not needed for interacting with the running daemon, avoid requiring it). Not sure if this is still a problem with how things are setup internally.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I change the comment to: Requiring bootstrap inline in components/libp2p to reduce the cli execution time?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that's really accurate though. In reality the whole libp2p component should be ignored in CLI, as I don't think it's used at all (unless I'm mistaken). I'd leave it for now, and we can clean that all up when we update libp2p configuration.

@vasco-santos vasco-santos mentioned this pull request Jul 20, 2020
Merged
@vasco-santos vasco-santos force-pushed the chore/update-libp2p-runtime-config branch from 8d24506 to 9fe4afb Compare July 20, 2020 11:38
@vasco-santos vasco-santos marked this pull request as ready for review July 20, 2020 11:39
achingbrain
achingbrain suggested changes Jul 21, 2020
View reviewed changes
packages/ipfs/src/core/runtime/libp2p-browser.js Outdated
@@ -35,13 +35,13 @@ module.exports = () => {
config: {
peerDiscovery: {
autoDial: true,
// Optimization
// Requiring bootstrap inline in components/libp2p to reduce the cli execution time
Copy link
Member

Choose a reason for hiding this comment

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

This is browser config, the reference to cli execution time isn't relevant here.

@vasco-santos vasco-santos requested a review from achingbrain July 21, 2020 09:56
@achingbrain
Copy link
Member

Tests are failing:

ipfs: FAILED TESTS:
ipfs:   libp2p customization
ipfs:     options
ipfs:       ✖ should use options by default
ipfs:         Chrome Headless 84.0.4147.89 (Linux x86_64)
ipfs:       AssertionError: expected { Object (dht, peerDiscovery, ...) } to have deep property 'peerDiscovery' of { Object (autoDial, bootstrap, ...) }, but got { Object (autoDial, bootstrap, ...) }
ipfs:       + expected - actual
ipfs:          }
ipfs:          "mdns": {
ipfs:            "enabled": false
ipfs:          }
ipfs:       -  "undefined": {
ipfs:       -    "enabled": true
ipfs:       -  }
ipfs:          "webRTCStar": {
ipfs:            "enabled": false
ipfs:          }
ipfs:        }

Is the .tag property missing from one of the peer discovery modules? Does a dep need updating?

@vasco-santos
Copy link
Member Author

Yeah, we cannot use the tag in webrtc after all 😞
https://github.com/libp2p/js-libp2p-webrtc-star/blob/master/src/index.js#L57
We need the instance of webrtc to access it. All the other discovery have it exported, but as webrtc is also a transport it is inside the discovery scope.

I will have this stuff properly addressed when we improve the libp2p config. For now we can include a comment to let users know where the tag comes from

@vasco-santos vasco-santos requested review from achingbrain and removed request for achingbrain July 23, 2020 16:22
achingbrain
achingbrain approved these changes Jul 24, 2020
View reviewed changes
Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

LGTM

@achingbrain achingbrain merged commit b450890 into master Jul 24, 2020
@achingbrain achingbrain deleted the chore/update-libp2p-runtime-config branch July 24, 2020 09:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jacobheun jacobheun jacobheun left review comments

@achingbrain achingbrain achingbrain approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vasco-santos @achingbrain @jacobheun