remixz / messenger-bot

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

Promise API #54

Closed IcanDivideBy0 closed 7 years ago

IcanDivideBy0 commented 7 years ago

fixes #52

eXeDK commented 7 years ago

I haven't tested this yet but from a quick glance shouldn't it just use Promises and not Promises and callbacks?

IcanDivideBy0 commented 7 years ago

Well @remixz asked for a non breaking change, so callbacks are still supported: the requests are now managed by promises, which will then invoke callbacks if provided. The promise is always returned (an async function with callback will never have anything meaningful to return anyways, simpler to always return promise)

IcanDivideBy0 commented 7 years ago

Maybe I should switch to request-native-promise if you guys want to avoid bluebird dependency ?

eXeDK commented 7 years ago

Aren't you only returning promises on reject? As I see it the continued use of callbacks together with promises on rejects seems silly.

I don't know about @remixz but I prefer bluebird over native promises.

IcanDivideBy0 commented 7 years ago

Hmm promise are always returned, on reject and fulfil. This pattern works like a charm, what's so "silly" ? If you don't get it, you still can test it yourself:

const httpResult = {
  error: null,
  foo: 'bar'
}

function test(cb) {
  return Promise.resolve(httpResult)
  // return Promise.reject('ERROR')
  .then(body => {
    if (body.error) return Promise.reject(body.error)
    if (!cb) return body
    cb(null, body)
  })
  .catch(err => {
    if (!cb) return Promise.reject(err)
    cb(err)
  })
}

test().then(console.log).catch(console.error)
test(console.log)
IcanDivideBy0 commented 7 years ago

As of bluebird, i see it as an unnecessary orverhead for such a lib, but does not matters that much

eXeDK commented 7 years ago

Seems I've missed the Promise.resolve twice for some reason. Sorry about that ;-)

flore2003 commented 7 years ago

Any news on this? Looks like a good addition and works like a charm

eXeDK commented 7 years ago

Maybe @remixz has some comments?

eXeDK commented 7 years ago

I think time is due to merge this since @remixz has yet to comment on it.

Have a great Christmas everyone