Skip to content
This repository was archived by the owner on Mar 10, 2020. It is now read-only.

refactor: convert bootstrap API to async/await #1154

Merged
merged 2 commits into from
Nov 14, 2019
Merged

refactor: convert bootstrap API to async/await #1154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ae4142b
refactor: convert bootstrap API to async/await
Nov 12, 2019
ea05976
fix: apparently delete does not work in browser
Nov 12, 2019
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
46 changes: 21 additions & 25 deletions src/bootstrap/add.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,32 +1,28 @@
'use strict'

const promisify = require('promisify-es6')
const Multiaddr = require('multiaddr')
const configure = require('../lib/configure')

module.exports = (send) => {
return promisify((args, opts, callback) => {
if (typeof opts === 'function' &&
!callback) {
callback = opts
opts = {}
module.exports = configure(({ ky }) => {
return async (addr, options) => {
if (addr && typeof addr === 'object' && !Multiaddr.isMultiaddr(addr)) {
Copy link
Collaborator

@achingbrain achingbrain Nov 14, 2019

Choose a reason for hiding this comment

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

Are we trying to support an options object as the first argument? If so, is this necessary? The spec says addr is not optional.

Copy link
Contributor Author

@alanshaw alanshaw Nov 14, 2019

Choose a reason for hiding this comment

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

It's what the current code does. So you can pass add(null, { default: true }) or add({ default: true }) to add the default bootstrappers. Same sort of situation for rm (with all option).

So addr is required unless you're passing the default option.

Copy link
Contributor Author

@alanshaw alanshaw Nov 14, 2019

Choose a reason for hiding this comment

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

ipfs-inactive/interface-js-ipfs-core#557

Copy link
Contributor

@hugomrdias hugomrdias Nov 14, 2019

Choose a reason for hiding this comment

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

IMO we should leave it like this in this PR.

But i agree with alex and we should make an issue to improve this in the future cause from https://github.com/ipfs/interface-js-ipfs-core/blob/master/SPEC/BOOTSTRAP.md#ipfsbootstrapaddaddr-options i would understand i could do add({ default: true }) maybe we can simplify to:
if first param is undefined use defaults.

Copy link
Collaborator

@achingbrain achingbrain Nov 14, 2019

Choose a reason for hiding this comment

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

Ah, yes, quite right. Sigh.

Copy link
Contributor Author

@alanshaw alanshaw Nov 14, 2019

Choose a reason for hiding this comment

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

But i agree with alex and we should make an issue to improve this in the future

❤️ me too. I'm just trying to get everything switched to async/await first and then take a second pass at API changes.

options = addr
addr = null
}

// opts is the real callback --
// 'callback' is being injected by promisify
if (typeof opts === 'function' &&
typeof callback === 'function') {
callback = opts
opts = {}
}
options = options || {}

if (args && typeof args === 'object') {
opts = args
args = undefined
}
const searchParams = new URLSearchParams(options.searchParams)
if (addr) searchParams.set('arg', `${addr}`)
if (options.default != null) searchParams.set('default', options.default)

const res = await ky.post('bootstrap/add', {
timeout: options.timeout,
signal: options.signal,
headers: options.headers,
searchParams
}).json()

send({
path: 'bootstrap/add',
args: args,
qs: opts
}, callback)
})
}
return res
}
})
16 changes: 6 additions & 10 deletions src/bootstrap/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,9 @@
'use strict'

const moduleConfig = require('../utils/module-config')
const callbackify = require('callbackify')

module.exports = (arg) => {
const send = moduleConfig(arg)

return {
add: require('./add')(send),
rm: require('./rm')(send),
list: require('./list')(send)
}
}
module.exports = config => ({
add: callbackify.variadic(require('./add')(config)),
rm: callbackify.variadic(require('./rm')(config)),
list: callbackify.variadic(require('./list')(config))
})
28 changes: 15 additions & 13 deletions src/bootstrap/list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
'use strict'

const promisify = require('promisify-es6')
const configure = require('../lib/configure')

module.exports = (send) => {
return promisify((opts, callback) => {
if (typeof (opts) === 'function') {
callback = opts
opts = {}
}
send({
path: 'bootstrap/list',
qs: opts
}, callback)
})
}
module.exports = configure(({ ky }) => {
return async (options) => {
options = options || {}

const res = await ky.get('bootstrap/list', {
timeout: options.timeout,
signal: options.signal,
headers: options.headers,
searchParams: options.searchParams
}).json()

return res
}
})
46 changes: 21 additions & 25 deletions src/bootstrap/rm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,32 +1,28 @@
'use strict'

const promisify = require('promisify-es6')
const Multiaddr = require('multiaddr')
const configure = require('../lib/configure')

module.exports = (send) => {
return promisify((args, opts, callback) => {
if (typeof opts === 'function' &&
!callback) {
callback = opts
opts = {}
module.exports = configure(({ ky }) => {
return async (addr, options) => {
if (addr && typeof addr === 'object' && !Multiaddr.isMultiaddr(addr)) {
options = addr
addr = null
}

// opts is the real callback --
// 'callback' is being injected by promisify
if (typeof opts === 'function' &&
typeof callback === 'function') {
callback = opts
opts = {}
}
options = options || {}

if (args && typeof args === 'object') {
opts = args
args = undefined
}
const searchParams = new URLSearchParams(options.searchParams)
if (addr) searchParams.set('arg', `${addr}`)
if (options.all != null) searchParams.set('all', options.all)

const res = await ky.post('bootstrap/rm', {
Copy link
Collaborator

@achingbrain achingbrain Nov 14, 2019

Choose a reason for hiding this comment

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

Looking at the commit history, what happened with DELETE here? Our HTTP API is all POST, all the time so this is fine like this, but that aside I thought DELETE was ok to use in browsers?

Copy link
Contributor Author

@alanshaw alanshaw Nov 14, 2019

Choose a reason for hiding this comment

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

I couldn't be bothered to dig in at the time but thinking about it now it's because browsers are respecting CORS and our default ipfsd-ctl config does not include DELETE https://github.com/ipfs/js-ipfsd-ctl/blob/7928c1eee092a9b96a59328294c84c9f065217a3/src/defaults/config.json#L5-L9

Copy link
Contributor

@hugomrdias hugomrdias Nov 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Do you want me to add DELETE to ctl? ?
In this case i would use POST cause its more RPC than REST
i would only use DELETE if the url was https://host/bootstrap/{addr}

Copy link
Collaborator

@achingbrain achingbrain Nov 14, 2019

Choose a reason for hiding this comment

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

I'm not suggesting changing this to DELETE, I just wanted to know what happened 😉

timeout: options.timeout,
signal: options.signal,
headers: options.headers,
searchParams
}).json()

send({
path: 'bootstrap/rm',
args: args,
qs: opts
}, callback)
})
}
return res
}
})
2 changes: 1 addition & 1 deletion src/utils/load-commands.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ function requireCommands (send, config) {
getEndpointConfig: require('../get-endpoint-config')(config),
bitswap: require('../bitswap')(config),
block: require('../block')(config),
bootstrap: require('../bootstrap')(config),
dag: require('../dag')(config)
}

Expand All @@ -110,7 +111,6 @@ function requireCommands (send, config) {
pin: require('../pin'),

// Network
bootstrap: require('../bootstrap'),
dht: require('../dht'),
name: require('../name'),
ping: require('../ping'),
Expand Down