fix: accept http client instance

[achingbrain]

Instead of the http client being a dependency of this module, accept
a pre-configured http client instance at construction time.

This resolves the weird dependency loop and means we can remove this
module from the no-hoist config in ipfs.

BREAKING CHANGE:

• An instance of ipfs http client is now a required argument

[jacobheun]

This resolves the weird dependency loop and means we can remove this
module from the no-hoist config in ipfs.

What's the hoist problem ipfs is hitting? The ipfs-http-client is still a peer dependency as they have to be installed together to work properly. Is this a problem with major version changes of the api still working with this module?

I assume this is something like: when ipfs-http-client@47 needs to be released, this module has to be updated for IPFS, but it requires this module to be released with the "unreleased" client version that's not ready yet?

[achingbrain]

The hoisting problem is that libp2p-delegated-* end up installed in /node_modules - they try to require('ipfs-http-client') but there's no /node_modules/ipfs-http-client as they do not declare a runtime dep so none is installed. There is a symlink from /packages/ipfs/node_modules/ipfs-http-client to /packages/ipfs-http-client however (the actual peer dep), so we add libp2p-delegated-* to the nohoist list which installs them at /packages/ipfs/node_modules instead.

This allows them to require('ipfs-http-client') but also means their dependencies are not hoisted either, so there's a duplicate peer-id module (for example) under /packages/ipfs/node_modules/libp2p-delegated-peer-routing/node_modules/peer-id which is the same version as the hoisted /node_modules/peer-id which is used by everything else in the stack.

This is primarily a dev problem and not something that occurs when people are using ipfs in their own projects, so I'd be inclined to ignore it and mentally make a note to PR lerna to symlink peer dependencies during hoisting, but the peer dependency thing doesn't really guarantee that you'll get the version you want so it's not safe and the versions lag often, printing warning messages when installing ipfs that confuse people and show that in actual fact these modules are not getting the version they want.

Since these modules also do deep requires into the internals of the http client, it prevents refactoring those internals without either releasing a major or otherwise inadvertently breaking the delegate modules. Now they depend only on the core-api which we hope is more stable over time.

The release dance is another problem, yes.

All in all it's a bit of a code smell.

[jacobheun]

Passing in a client makes complete sense to me and I'm in favor of that change.

For the peerDependency, one of the pain points we've been trying to work around with libp2p is declaring version matching with js-libp2p in particular. We encounter issues quite often where users are using versions of modules that don't work with the version of libp2p they're using. peerDependencies is a reasonable method for us to be able to declare that. Another option is to just declare this in the docs, but that is more prone to getting stale.

If we moved away from caret semver to range semver for peerDependencies this could avoid the warning for end users. "ipfs-http-client": ">=44.0.0" would allow for any future versions of the http client to not log warnings, and we would only need to update it when a breaking change occurs that affects this module. The warning will still happen for pre releases, because they don't apply to normal semver, but final releases wouldn't have the issue.

[achingbrain]

I think this is a hard problem to solve.

Marking the peer dep as >=44.0.0 will hide the 'you need to install a peer dep' warning but it makes no guarantee that 45.0.0 won't remove the .dht methods, for example.

You could use feature detection to ensure the right methods were present, but even then if the internal implementation is incompatible for whatever reason (e.g. different deps, or whatever) it could still explode at runtime.

[jacobheun]

LGTM, just one minor formatting issue in the readme