Skip to content

fix: accept http client instance #39

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Aug 26, 2020
Merged

fix: accept http client instance #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f847187
fix: accept http client instance
achingbrain Aug 25, 2020
7115adf
chore: demand client instance
achingbrain Aug 25, 2020
d0e30a2
chore: update node versions
achingbrain Aug 25, 2020
8f9bc5b
chore: update readme
achingbrain Aug 25, 2020
83000cf
chore: restore peer deps
achingbrain Aug 26, 2020
7016c4a
chore: update README.md
jacobheun Aug 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ stages:
- cov

node_js:
- '10'
- '12'
- 'lts/*'
- 'node'

os:
- linux
Expand Down
10 changes: 8 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ Requires access to `/api/v0/dht/findpeer` HTTP API endpoint of the delegate node

## Requirements

`libp2p-delegated-peer-routing` leverages the `ipfs-http-client` library and requires it as a peer dependency, as such, both must be installed in order for this module to work properly.
`libp2p-delegated-peer-routing` leverages the `ipfs-http-client` library and requires an instance of it as a constructor argument.

```sh
npm install ipfs-http-client libp2p-delegated-peer-routing
Expand All @@ -20,9 +20,15 @@ npm install ipfs-http-client libp2p-delegated-peer-routing

```js
const DelegatedPeerRouting = require('libp2p-delegated-peer-routing')
const ipfsHttpClient = require('ipfs-http-client')

// default is to use ipfs.io
const routing = new DelegatedPeerRouing()
const routing = new DelegatedPeerRouing(ipfsHttpClient({
// use default api settings
protocol: 'https',
port: 443,
host: 'node0.delegate.ipfs.io'
}))

try {
const { id, multiaddrs } = await routing.findPeer('peerid')
Expand Down
17 changes: 10 additions & 7 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,23 +22,26 @@
"coverage": "aegir coverage"
},
"devDependencies": {
"aegir": "^25.0.0",
"aegir": "^26.0.0",
"go-ipfs": "0.6.0",
"ipfs-http-client": "^45.0.0",
"ipfs-utils": "^2.2.0",
"ipfsd-ctl": "^5.0.0",
"ipfs-http-client": "^46.0.0",
"ipfs-utils": "^3.0.0",
"ipfsd-ctl": "^7.0.0",
"it-all": "^1.0.2"
},
"peerDependencies": {
"ipfs-http-client": "^44.0.0"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@achingbrain achingbrain Aug 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 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.

"dependencies": {
"cids": "^1.0.0",
"debug": "^4.1.1",
"p-defer": "^3.0.0",
"p-queue": "^6.3.0",
"peer-id": "^0.14.0"
},
"peerDependencies": {
"ipfs-http-client": "^46.0.0"
},
"browser": {
"go-ipfs": false
},
"contributors": [
"Jacob Heun <[email protected]>",
"Alex Potsides <[email protected]>",
Expand Down
30 changes: 15 additions & 15 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
'use strict'

const PeerId = require('peer-id')
const createFindPeer = require('ipfs-http-client/src/dht/find-peer')
const createQuery = require('ipfs-http-client/src/dht/query')
const CID = require('cids')
const { default: PQueue } = require('p-queue')
const defer = require('p-defer')
Expand All @@ -12,27 +10,29 @@ const log = debug('libp2p-delegated-peer-routing')
log.error = debug('libp2p-delegated-peer-routing:error')

const DEFAULT_TIMEOUT = 30e3 // 30 second default
const DEFAULT_IPFS_API = {
protocol: 'https',
port: 443,
host: 'node0.delegate.ipfs.io'
}
const CONCURRENT_HTTP_REQUESTS = 4

class DelegatedPeerRouting {
constructor (api) {
this.api = Object.assign({}, DEFAULT_IPFS_API, api)
this.dht = {
findPeer: createFindPeer(this.api),
getClosestPeers: createQuery(this.api)
constructor (client) {
if (client == null) {
throw new Error('missing ipfs http client')
}

this._client = client

// limit concurrency to avoid request flood in web browser
// https://github.com/libp2p/js-libp2p-delegated-content-routing/issues/12
this._httpQueue = new PQueue({
concurrency: CONCURRENT_HTTP_REQUESTS
})
log(`enabled DelegatedPeerRouting via ${this.api.protocol}://${this.api.host}:${this.api.port}`)

const {
protocol,
host,
port
} = client.getEndpointConfig()

log(`enabled DelegatedPeerRouting via ${protocol}://${host}:${port}`)
}

/**
Expand All @@ -55,7 +55,7 @@ class DelegatedPeerRouting {

try {
return await this._httpQueue.add(async () => {
const { addrs } = await this.dht.findPeer(idStr, {
const { addrs } = await this._client.dht.findPeer(idStr, {
timeout: options.timeout
})

Expand Down Expand Up @@ -102,7 +102,7 @@ class DelegatedPeerRouting {

const peers = new Map()

for await (const result of this.dht.getClosestPeers(keyStr, {
for await (const result of this._client.dht.query(keyStr, {
timeout: options.timeout
})) {
switch (result.type) {
Expand Down
66 changes: 28 additions & 38 deletions test/index.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const { createFactory } = require('ipfsd-ctl')
const PeerID = require('peer-id')
const { isNode } = require('ipfs-utils/src/env')
const concat = require('it-all')
const ipfsHttpClient = require('ipfs-http-client')

const DelegatedPeerRouting = require('../src')
const factory = createFactory({
Expand Down Expand Up @@ -72,50 +73,39 @@ describe('DelegatedPeerRouting', function () {
})

describe('create', () => {
it('should default to https://node0.delegate.ipfs.io as the delegate', () => {
const router = new DelegatedPeerRouting()

expect(router.api).to.include({
protocol: 'https',
port: 443,
host: 'node0.delegate.ipfs.io'
})
it('should require an http api client instance at construction time', () => {
expect(() => new DelegatedPeerRouting()).to.throw()
})

it('should allow for just specifying the host', () => {
const router = new DelegatedPeerRouting({
host: 'other.ipfs.io'
})

expect(router.api).to.include({
protocol: 'https',
port: 443,
host: 'other.ipfs.io'
})
})

it('should allow for overriding the api', () => {
const api = {
apiPath: '/api/v1',
it('should accept an http api client instance at construction time', () => {
const client = ipfsHttpClient({
protocol: 'http',
port: 8000,
host: 'localhost'
}
const router = new DelegatedPeerRouting(api)
})
const router = new DelegatedPeerRouting(client)

expect(router.api).to.include(api)
expect(router).to.have.property('_client')
.that.has.property('getEndpointConfig')
.that.is.a('function')

expect(router._client.getEndpointConfig()).to.deep.include({
protocol: 'http:',
port: '8000',
host: 'localhost'
})
})
})

describe('findPeers', () => {
it('should be able to find peers via the delegate with a peer id string', async () => {
const opts = delegatedNode.apiAddr.toOptions()

const router = new DelegatedPeerRouting({
const router = new DelegatedPeerRouting(ipfsHttpClient({
protocol: 'http',
port: opts.port,
host: opts.host
})
}))

const { id, multiaddrs } = await router.findPeer(peerIdToFind.id)
expect(id).to.exist()
Expand All @@ -125,11 +115,11 @@ describe('DelegatedPeerRouting', function () {

it('should be able to find peers via the delegate with a peerid', async () => {
const opts = delegatedNode.apiAddr.toOptions()
const router = new DelegatedPeerRouting({
const router = new DelegatedPeerRouting(ipfsHttpClient({
protocol: 'http',
port: opts.port,
host: opts.host
})
}))

const { id, multiaddrs } = await router.findPeer(PeerID.createFromB58String(peerIdToFind.id))
expect(id).to.exist()
Expand All @@ -140,11 +130,11 @@ describe('DelegatedPeerRouting', function () {

it('should be able to specify a timeout', async () => {
const opts = delegatedNode.apiAddr.toOptions()
const router = new DelegatedPeerRouting({
const router = new DelegatedPeerRouting(ipfsHttpClient({
protocol: 'http',
port: opts.port,
host: opts.host
})
}))

const { id, multiaddrs } = await router.findPeer(PeerID.createFromB58String(peerIdToFind.id), { timeout: 2000 })
expect(id).to.exist()
Expand All @@ -155,11 +145,11 @@ describe('DelegatedPeerRouting', function () {

it('should not be able to find peers not on the network', async () => {
const opts = delegatedNode.apiAddr.toOptions()
const router = new DelegatedPeerRouting({
const router = new DelegatedPeerRouting(ipfsHttpClient({
protocol: 'http',
port: opts.port,
host: opts.host
})
}))

// This is one of the default Bootstrap nodes, but we're not connected to it
// so we'll test with it.
Expand All @@ -172,11 +162,11 @@ describe('DelegatedPeerRouting', function () {
it('should be able to query for the closest peers', async () => {
const opts = delegatedNode.apiAddr.toOptions()

const router = new DelegatedPeerRouting({
const router = new DelegatedPeerRouting(ipfsHttpClient({
protocol: 'http',
port: opts.port,
host: opts.host
})
}))

const nodeId = await delegatedNode.api.id()
const delegatePeerId = PeerID.createFromCID(nodeId.id)
Expand All @@ -196,11 +186,11 @@ describe('DelegatedPeerRouting', function () {
it('should find closest peers even if the peer doesnt exist', async () => {
const opts = delegatedNode.apiAddr.toOptions()

const router = new DelegatedPeerRouting({
const router = new DelegatedPeerRouting(ipfsHttpClient({
protocol: 'http',
port: opts.port,
host: opts.host
})
}))

const nodeId = await delegatedNode.api.id()
const delegatePeerId = PeerID.createFromCID(nodeId.id)
Expand Down