Promises API

[victorb]

Most environments now support the more pleasant API of Promise ( https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm/Promise ), that is easier to handle and chain together. We should think about migrating to start using promises instead of callbacks where applicable.

List of browsers that supports it current: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
(IE doesn't support it + some olders versions of other browsers, probably have to provide an existing shim as well...)

In node, promises have been natively supported since 0.12 so shouldn't be an issue there.

[dignifiedquire]

I like promises, but I know some people get very angry if you tell them to move away from callbacks, so please lets not have big fight about it. Using bluebird it's very easy to expose a callback + promise api which I favor as it gives people the ability to use what they prefer.

[RichardLitt]

I agree that this doesn't need to be prioritized. If it works, let it stay as it is.

[harlantwood]

Bluebird's promisifyAll works perfectly here. I only use this lib with promises, eg: https://github.com/nodesphere/nodesphere/blob/1479f40cef4824e3135a20f65889a8110d0fc042/lib/adaptor/ipfs.coffee#L26-L31

[victorb]

@dignifiedquire hah, people that get angry over programming things always amazes me. Anyways, callback+promises might be the way to go forward, if Bluebird makes that easy and is compatible with native Promise.

@RichardLitt for sure, cosmetic change at most but still valuable one when you're building something bigger than just using ipfs.add.

@harlantwood Oh, didn't know about Bluebird's promisifyAll, is using native promises in the background? Nice reference though, seems simple enough.

[RichardLitt]

@victorbjelkholm:

cosmetic change

Bingo. Promises and Callbacks are the higher-level equivalent of semicolons and no semicolons. Best to avoid entirely if you're not doing a major refactor with new team.

[victorb]

@RichardLitt Well, it being a cosmetic thing doesn't mean that you can ignore it completely. You want people to use a library? Make sure the API for using it is simple and up-to-date with the rest of the ecosystem.

I didn't open up this issue to propose a change of this right now. But I'm sure the library would be more pleasant for people to use, if in the future, we can provide something like what @harlantwood suggested above. But please, is not the end of the world if it doesn't exists.

[bcomnes]

I don't like promises very much, and using them doesn't usually make my life more pleasant. I don't care if promises are supported, as long as I get a callback api without having to wrap promises to not use them.

[travisperson]

As much as I love bluebird, I would not use it in this lib unless you have very strong reasons to use it instead of something that can be removed for native promises.

Also see: https://github.com/HenrikJoreteg/native-promisify-if-present

[harlantwood]

Thanks @travisperson. that library, or a similar solution, looks like a non-intrusive solution that gives everyone what they want -- callback API and promise API out of the box.

To clarify my earlier comment, I wasn't suggesting adding bluebird to this library (node-ipfs-api) -- I was pointing out that as a user of the library, I am consuming a promise API right now, thanks to the help of Bluebird's awesome promisifyAll method. My code looks something like:

Promise = require 'bluebird'
ipfsApi = require 'ipfs-api'
ipfs = Promise.promisifyAll ipfsApi 

At this point, in addition to the usual ipfs.add and ipfs.ls methods which take callbacks, I also have ipfs.addAsync and ipfs.lsAsync, which return promises. Sample usage:

  put: ({content}) ->
    @ipfs.addAsync new @ipfs.Buffer content
      .then (items) =>
        for item in items
          item.Hash
      .catch (err) ->
        console.error err

  fetch: ({rootNodeId}) ->
    @ipfs.lsAsync rootNodeId
    .then (response) =>
      data = response.Objects[0]  
      @process {rootNodeId, data}
    .catch (err) ->
      console.error err

[daviddias]

I'm all in on having a promises API if the users of node-ipfs-api are looking for it. However I don't think there will be any valid reason to drop callbacks in favor to promises.

If someone wants to volunteer to make that really nice promises API along the side of the callbacks API (+ tests for it), let's do it :)

[bcomnes]

On further reading of my prior comment, sorry for sounding like a dick ;P Let me try that again.

Adding a promise api would be great! As long as it doesn't degrade the callback api in any way.

[jbenet]

@harlantwood pointed out that users can just promisify the API trivially easy. I would think that's the best route, leaving it to the user, and maybe have a comment in the readme. It keeps our interface smaller and supports both methods.

—
Sent from Mailbox

On Thu, Oct 22, 2015 at 6:41 PM, David Dias [email protected]
wrote:

I'm all in on having a promises API if the users of node-ipfs-api are looking for it. However I don't think there will be any valid reason to drop callbacks in favor to promises.

If someone wants to volunteer to make that really nice promises API along the side of the callbacks API (+ tests for it), let's do it :)

Reply to this email directly or view it on GitHub:
#80 (comment)

[dignifiedquire]

Actually at the moment bluebirds Promise.promisifyAll doesn't work properly, as it tries to go up the prototype chain to promisify nested methods. But at the moment the implementation does not use the prototype chain, which has the result that this will fail:

const API = Promise.promisifyAll(require('ipfs-api'))

const api = new API()

api.idAsync().then(id => console.log(id))
// => some id

api.swarm.peersAsync().then(peers => console.log(peers))
// => undefined method peersAsync

[dignifiedquire]

After some more investigation it looks like we can not expose our API easily in a way that Promise.promisifyAll(require('ipfs-api')) will work, as we are using nested objects, and those are skipped by this promisification call.
My suggestion is to go the way that is best for the consumers and expose an api that does callbacks and promises at the same time. This would work like this:

const API = require('ipfs-api')
const api = new API('127.0.0.1:5001')

api.id()
  .then(id => console.log(id))
  .catch(err => console.error(err))

api.id(function (err, id) {
  if (err) return console.error(err)

  console.log(id)  
})

To expose this kind of API we just need to switch the internal generation calls to using promises and then call Promise.nodeify on them, using the Promise package, which is small and node + browser compatible.

[daviddias]

I've a question (following our IRC discuss)

Following nodeify example, how can I return a value from inside a callback within the callback? e.g:

module.exports = Promise.nodeify(awesomeAPI)

function awesomeAPI(a, b) {
  oneLevelDeep (a, b, function (err, res) {
    return res // ?? Would this even work?
  })
}

Another example, taking the .id call

Currently

self.id = function (id, cb) {
  if (typeof id === 'function') {
    cb = id
    id = null
  }
  requestAPI('id', id, null, null, cb)
}

With nodeify, it would be

self.id = function (id) {
  if (typeof id === 'function') {
    cb = id
    id = null
  }
  requestAPI('id', id, null, null)
}

But how would requestAPI pass the value to whom calls .id()?

[dignifiedquire]

Example implmentation: https://github.com/Dignifiedquire/js-ipfs-api/blob/es2015-promises/src/index.js#L88-L95

[daviddias]

After the several discussions we had so far, I'm understanding that this topic, promises vs callbacks, is way bigger than a simply binary decision or even a compromise between the two.

In order to support both APIs the code has be developed using promises and then put a 'callbacks lid' on top so that users can still use callbacks. This could work, but every contributor would have to buy in on promises, which could be ok, however, developing with promises in Node.js (non browser apps), can be a pain because Node.js simply doesn't have a promises API (or a 'promise' of having it in the near future) and it async control flow will be messed up.

We could also avoid promises and continue using only callbacks (so far, it has worked well for a ton of JS applications), but as @dignifiedquire pointed out, every library and browser API is moving to promises, specially the React stuff we even use ourselves.

This leads me to think that this universal/isomorphic javascript is just an idea built on weak foundations and that we will need to accept having on JS way for browser code and one JS way for Node.js code.

[dignifiedquire]

After a hangout with @diasdavid we have come to the conclusion that we do not want to change all the modules to another async handling style, simply because it would be too much work better spent otherwise. So for all node code we will stick with callbacks, but we want to try to make it possible to easily expose a promisifable version of the API using tools like bluebirds promisfiyAll.

As that is not usable at the moment we are searching for the best way to do that and I have opened petkaantonov/bluebird#840 asking for help, but please feel free to add any suggestions here.