feat: enable peer discovery modules by default

[alanshaw]

This PR will enable any provided peer discovery modules by default if no configuration for the module is supplied/needed.

As before, modules can be explicitly disabled or enabled by passing config.

This enables pre-configured modules (instances) to be passed and enabled without them having to have a tag and an unused config section.

[daviddias]

I preferred being explicit but this is good as well. Mind adding tests?

[jacobheun]

@alanshaw if we're going to change the defaults I think we should change them in the ./src/config.js file.

With this change the config will throw an error if someone doesn't supply the config for the peerDiscovery, tests would be great here to catch that. We should set default values here:

js-libp2p/src/config.js

Lines 66 to 72 in 6905f1b

	config.modules.peerDiscovery.forEach((discovery) => {
	// If it's a function, validate we have configs for it
	if (typeof discovery === 'function') {
	Joi.reach(schema, 'config.peerDiscovery').keys({
	[discovery.tag]: Joi.object().required()
	})
	}

. This also prevents us from needing to do null checks in the constructor.

[alanshaw]

@jacobheun I don't think that peer discovery module validation code is working properly 😟 . I just found problems with the test for it and fixed them and it fails now:

f753d61

I'm not changing any defaults, just allowing modules that don't require any config or have already been configured to still be registered and started. IMHO it's unnecessary to require a developer to pass a preconfigured module instance and also have to set a config value with { enabled: true } somewhere else in the options. I think it is a reasonable expectation that if I pass a module and say nothing about whether it is enabled or disabled then I want it to be enabled.

I don't think we should validate config for each of the discovery modules - that's outside of the scope of this module. We can't know what all the config options are and keep that info up to date. All we could meaningfully do would be to validate a boolean enabled property, which I think should just be optional anyway - hence this PR.

[alanshaw]

@jacobheun how about now? Is better 😛 ?

[jacobheun]

@alanshaw yesssss! This looks great, and thank you for cleaning up the config, so much easier to read. I agree that defaulting to enabled will be easier to users.

Failed ci looks to be CI having a bad day, not an issue with tests.

[daviddias]

LGTM. Mind rebasing master onto this branch and wait for CI before the merge?