remixz / messenger-bot

A Node client for the Facebook Messenger Platform
MIT License
1.09k stars 213 forks source link

Can we Promisify the API? #18

Closed albert-tse closed 8 years ago

albert-tse commented 8 years ago

This is getting close to callback spaghetti: echo example

kittrCZ commented 8 years ago

3 levels of nesting? That's not so terrible :)

remixz commented 8 years ago

Using Promises doesn't inherently solve the problem of so-called "callback hell." You could chain a bunch of promises together, and it wouldn't look any better than having nested callbacks, aside from there being a bit less indentation. This is a good resource on how to manage callbacks effectively: http://callbackhell.com/

It also would be hard to use Promises with this module, since it's mostly an event based module. I can't think of a reasonable way to use Promises with events. bot.on('message').then((payload, reply) => {}) doesn't make sense to me, and just passing a Promise as the second param doesn't seem any different than just passing a callback function. If you don't like the nesting, just hoist the function up like this:

function messageHandler (payload, reply) {
  let text = payload.message.text

  bot.getProfile(payload.sender.id, (err, profile) => {
    if (err) throw err

    reply({ text }, (err) => {
      if (err) throw err

      console.log(`Echoed back to ${profile.first_name} ${profile.last_name}: ${text}`)
    })
  })
}

bot.on('message', messageHandler)

As well, if the module were to start using Promises for events, it'd break the conventions of events in Node, and would require custom implementation. It's a lot nicer to just use the events implementation from Node core, since it's very well tested, and people are likely to already be familiar with it if they've worked with other modules in the Node ecosystem.

I also have issues with how Promises are implemented in terms of error catching, and them failing silently if you forget to catch an error. Sure, this can happen in callbacks too, but at least with most callbacks, you're forced to write in the err param, which puts it in your head that you should probably be ready to handle an error.

To sum it up: No, I won't be refactoring the API to use Promises. It'd be, in my opinion, an unnecessary breaking change to the module, since it would break Node conventions and potentially harm user's experience. I think that callbacks can be managed just as well as Promises, if not better because of modules like async. If you really care about Promises, feel free to fork this module to use them, or use something like bluebird and its promisification: http://bluebirdjs.com/docs/api/promisification.html