fix: validate list #97
Conversation
| * | ||
| */ | ||
| constructor (options) { | ||
| constructor (options = {}) { | ||
| assert(options.list && options.list.length, 'Bootstrap requires a list of peer addresses') |
There was a problem hiding this comment.
I think this is going to create problems for people. I've seen several configs from people getting started where they include a bunch of the modules but not all the options, such as the bootstrap list. If we had warn level logging I think my preference would be to do that, since we don't I think logging that nothing was provided and not throwing might be best for usability. There's definitely another argument here about making libp2p easier to configure for less advanced users, but we can discuss that elsewhere.
There was a problem hiding this comment.
I can change to a warning for now.
My main reason to this change instead of log, if someone adds Bootstrap without any peers, this module will blindly not do nothing (unless someone goes through the logs). Ideally, we would avoid this type of behavior
There was a problem hiding this comment.
Perhaps we can just update the assert error to be more specific about how to fix the problem?
There was a problem hiding this comment.
What do you think on: 'Bootstrap requires a list of peer addresses on its configuration options'
I would like to go on more detail, but feels out of scope to explain libp2p specificity here
There was a problem hiding this comment.
Actually, nvm, I think this should be solved in libp2p itself and the assert here is fine. The reason for this is that how bootstrap is configured here is not how users configure it in libp2p as it as modules vs config options.
Aside from the package.json conflict this looks good.
vasco-santos
force-pushed
the
fix/validate-list
branch
from
23b4780
to
684054f
Compare
if the list was not provided before, we would have a runtime error