Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

Promisify stop()? #1076

Closed
haadcode opened this issue Nov 14, 2017 · 7 comments
Closed

Promisify stop()? #1076

haadcode opened this issue Nov 14, 2017 · 7 comments
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue

Comments

@haadcode
Copy link
Member

We currently return from stop() with a callback.

In order to stop IPFS properly, we write:

ipfs.stop(() => {
// do other shutdown things
})

I find myself often preferring to use Promises and more recently await. As per IPFS api functions, we return a Promise (or callback) and I find it confusing that stop() is only callback.

If we returned a Promise, we would write:

ipfs.stop().then(() => {
// do other shutdown things
})

// Or:
await ipfs.stop()
// do other shutdown things

I'd like to propose we promisify the stop function.

We would keep the original callback in addition to returning a Promise, same as we do for API functions and the start() function.

How does everyone feel about this?

I'd be happy to PR this, but this would be a great task for first contributors if someone wants to work on it!

@haadcode haadcode added exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue labels Nov 15, 2017
@mitra42
Copy link

mitra42 commented Nov 16, 2017

Absolutely - the functions with only callbacks add substantial weirdness to building anything on top of it. (Block.get and Block.put are two others that I have to promisify in my code).

@daviddias
Copy link
Member

@haadcode sounds good! Seems that we missed that one. Wanna submit that as a PR?

@daviddias
Copy link
Member

@mitra42 just noticed that indeed block.get and block.put are not promisified, wanna add a PR with that? https:/ipfs/js-ipfs/blob/master/src/core/components/block.js#L11-L15

@mitra42
Copy link

mitra42 commented Nov 16, 2017

All we are doing is

    _makepromises() {
        this.promisified = { ipfs: { block: {
            put: promisify(this.ipfs.block.put),
            get: promisify(this.ipfs.block.get)
        }}}
    }

Then call

await this.promisified.ipfs.block.get(cid)

as part of the workaround to the bug in #1049
Which i don't think is what you want for a PR. (I'm assuming what you want is something that tests for existence of a callback and returns a promise in its absence ?

Anyway ... I haven't done any PRs for IPFS because given all the multiple repositories etc I have zero confidence in finding accurate docs to do the full cycle - clone; edit; test; submit PR, especially not to do something that breaks other code elsewhere in IPFS.

@haadcode
Copy link
Member Author

PR for promisifying stop is here #1082. cc @diasdavid

@haadcode
Copy link
Member Author

@mitra42 @diasdavid I went ahead and PRed the promisified block functions here #1085. We also need updated tests from ipfs-inactive/interface-js-ipfs-core#170 to merge it.

@daviddias
Copy link
Member

thank you so much @haadcode 🌟

@mitra42 if you happen to have the intention to try, I'm happy to provide you pointers and help when you get blocked. Right now, you can check how @haadcode submitted this PRs so that you can learn what was necessary.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue
Projects
None yet
Development

No branches or pull requests

3 participants