refactor: convert bootstrap API to async/await #1154
Conversation
License: MIT Signed-off-by: Alan Shaw <[email protected]>
alanshaw
force-pushed
the
refactor/bootstrap-async-await
branch
from
e26688b to
ea05976
Compare
|
ping @hugomrdias @achingbrain 🙏 |
| opts = {} | ||
| module.exports = configure(({ ky }) => { | ||
| return async (addr, options) => { | ||
| if (addr && typeof addr === 'object' && !Multiaddr.isMultiaddr(addr)) { |
There was a problem hiding this comment.
Are we trying to support an options object as the first argument? If so, is this necessary? The spec says addr is not optional.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah, yes, quite right. Sigh.
There was a problem hiding this comment.
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.
| if (addr) searchParams.set('arg', `${addr}`) | ||
| if (options.all != null) searchParams.set('all', options.all) | ||
|
|
||
| const res = await ky.post('bootstrap/rm', { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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}
There was a problem hiding this comment.
I'm not suggesting changing this to DELETE, I just wanted to know what happened 😉
| if (addr) searchParams.set('arg', `${addr}`) | ||
| if (options.all != null) searchParams.set('all', options.all) | ||
|
|
||
| const res = await ky.post('bootstrap/rm', { |
There was a problem hiding this comment.
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}
| opts = {} | ||
| module.exports = configure(({ ky }) => { | ||
| return async (addr, options) => { | ||
| if (addr && typeof addr === 'object' && !Multiaddr.isMultiaddr(addr)) { |
There was a problem hiding this comment.
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.
No description provided.