mast / telegram-bot-api

First Telegram Bot API node.js library
http://mast.github.io/telegram-bot-api/
MIT License
246 stars 64 forks source link

Added error handling inside promise #34

Closed Christilut closed 7 years ago

Christilut commented 8 years ago

This will throw errors that are unhandled inside .then(), right now they are silently swallowed which is really nasty.

Also my editor has removed some whitespace, sorry about that but should be no big deal. The only actual change is:

var Promise = require('bluebird');

Promise.onPossiblyUnhandledRejection(function(error) {
  throw error;
})
jkantr commented 8 years ago

I agree on upgrading the promise lib to bluebird. I don't necessarily agree on this global error handling, feels like a hack. The user can just Promise.try on their side if they want to roll up all errors.

Can you give an example of an internal error that would be swallowed and not bubble to .then().catch()? Are we referring to the update loop? Because I've never experienced this issue

Christilut commented 8 years ago
bot.sendMessage({
  chat_id: CHANNEL_ID,
  text: 'test'
}).then(function() {
  console.log('Sent message')
  throw new Error('error message')
})

This error won't be shown currently. With the onPossiblyUnhandledRejection it will simply throw like expected.

jkantr commented 8 years ago
bot.sendMessage({
  chat_id: CHANNEL_ID,
  text: 'test'
}).then(function() {
  console.log('Sent message')
  throw new Error('error message')
}).catch(function(err) {
  // uhm.. pretty sure this will handle it just fine..
})
Christilut commented 8 years ago

Yea but then you are catching it explicitly. If you dont use catch then all errors are swallowed silently. So you have to know that you must use catch() or your code might be erroring at some point that you can't see... don't you think thats kind of bad?

Christilut commented 8 years ago

Also another point is that, as a user of this library, I can't choose to obtain this behavior. Adding the above bluebird snippet won't work because this project isn't using bluebird.

So switching to bluebird would allow a user to opt-in for this behavior, which is good. Although personally I think it should be default behavior, but thats preference.

jkantr commented 8 years ago

Ok, I understand what you're saying. I specifically wrap the majority of my promises in Bluebird.try() to avoid this exact scenario when dealing with multiple promises, so I guess it's not that much different. I suppose this comes from the current state of promises and the lack of obviousness of how rejection chains work.

As for switching to bluebird I was always 100% behind you. Its just the best promise lib out there by a mile.

Alright I like it.

derhuerst commented 8 years ago

Is there a specific reason for switching to Bluebird? "Its just the best promise lib out there by a mile." doesn't seem like a real one. What about native Promises?

jkantr commented 8 years ago

Roughly 3-4 times speed and memory optimization, bluebird is a beast: https://github.com/petkaantonov/bluebird/tree/master/benchmark

Native is slightly better since then but not much. Also the feature he's suggesting we implement is specific to bluebird (onPossiblyUnhandledRejection) along with opening up a bunch of other enhancements like promisify'd map/filter/reduce etc.

mast commented 7 years ago

Agree with fixes, thank you @Christilut