Promise not returning value

Question

I have a node issue where I'm using the imap module, and I am trying to return a promise. I have created a Promise.method() using bluebird. The code is actually available at: https://github.com/dkran/email-thinky-test/

So my issue is I have this file performing this:

//file: ./imap/helper.js 
var Imap = require('imap'),
    Promise = require('bluebird'),
    imap = require('../util/imap');

exports.parseMailbox = Promise.method(function(boxName){
  boxName = boxName || 'INBOX'
  imap.openBoxAsync(boxName, false).then(function(inbox){
    var messages = [], newMessage = {};
    imap.searchAsync(['ALL']).then(function(results){
      var f = Promise.promisifyAll(imap.fetch(results, {
        bodies: ['HEADER.FIELDS (FROM TO CC BCC SUBJECT)','TEXT'],
        struct: true
      }));

      f.on('message', function(msg, seqno) {
        newMessage = {}

        msg.on('body', function(stream, info) {
          //When we get a message, append the header to header, text to body.
          stream.on('data', function(chunk){
            if (info.which !== 'TEXT')
              newMessage.rawHeader += chunk.toString('utf8')
            else
              newMessage.body += chunk.toString('utf8')
          })

          //When stream is done, strip the unparsable characters at the beginning before parsing.
          //NOTE: I'm not actually sure what these unparseable characters actually are
          //but this all works kosher.
          stream.once('end', function() {
            if (info.which !== 'TEXT'){
              newMessage.rawHeader = newMessage.rawHeader.replace(/^undefined/, '')
              newMessage.header = Imap.parseHeader(newMessage.rawHeader)
            }
            if(newMessage.body)
              newMessage.body = newMessage.body.replace(/^undefined/, '')
          })
        })

        msg.once('attributes', function(attrs) {
          newMessage.attrs = attrs
        })

        msg.once('end', function() {
          messages[seqno-1] = newMessage
        })

      });

      f.once('error', function(err) {
        throw err
      });

      return f.onceAsync('end').then(function() {
        console.log('Messages: ' + messages.length)
        return messages
      })
    }).catch(function(e){
      throw e
    })
  }).catch(function(e){
    throw e
  })
})

module.exports = exports

and then another little file doing this:

//file: ./imap/index.js
var imap = require('../util/imap'),
  mail = require('./mail'),
  mailHelpers = require('./helpers');

imap.onceAsync('ready').then(function(){
  imap.getBoxesAsync().then(function(result){
    for(var box in result){
      mail.mailboxes.push(box)
    }
  }).catch(function(e){
    console.log(e)
  }).finally(function(){
    mailHelpers.parseMailbox('INBOX').then(function(res){
      mail.mail = res
      console.log('res: ' + res)
      console.log('mail: ' + mail.mail)
      console.log(mail.mailboxes)
    }).catch(function(e){
      console.log(e)
    })
  })
})

At the bottom of the helper file when the 'end' event triggers, and the console.log goes off, it says I have 9 messages (which is true.) if I log the messages there instead it shows them all in the object. but they never get returned it's always undefined. Why is this? All of my other callbacks I converted seemed to work fine.

exact output I get is:

res: undefined

mail: undefined

[ 'INBOX', 'Sent Messages', 'Deleted Messages', 'Drafts' ]

Messages: 9

You're trying to return a promise? Where? I'm seeing surprisingly few `return` statements for that in your code. — Bergi, Aug 07 '15 at 02:56
`return f.onceAsync('end').then(function() { console.log('Messages: ' + messages.length) return messages })` just at the end where I want it to return? — dkran, Aug 07 '15 at 02:57
Btw, `f.once('error', function(err) { throw err });` is not doing what you expect, and `.catch(function(e){ throw e })` can be completely omitted. — Bergi, Aug 07 '15 at 02:59
That's not the end, that's inside of two promise callbacks (`imap.openBoxAsync(boxName, false).then(…)` and `imap.searchAsync(['ALL']).then(…)`). The results of both ones you don't return. — Bergi, Aug 07 '15 at 03:01
ohhhh. I have to return a whole chain of promises. or else it won't work. — dkran, Aug 07 '15 at 03:27
Yes, you have to `return` from every function whose result (if its a value, a promise for a value, or a promise for nothing (but the time)) you want. Including `then` callback functions. — Bergi, Aug 07 '15 at 03:54

score 0 · Answer 1 · answered Aug 07 '15 at 04:58

You don't want to use Promise.method in this situation. Just return a promise that gets resolved somewhere along all of those callbacks.

//file: ./imap/helper.js 
var Imap = require('imap'),
  Promise = require('bluebird'),
  imap = require('../util/imap');

exports.parseMailbox = function(boxName) {
  return new Promise(function(resolve, reject) {

    boxName = boxName || 'INBOX'
    imap.openBoxAsync(boxName, false).then(function(inbox) {
      var messages = [],
        newMessage = {};
      imap.searchAsync(['ALL']).then(function(results) {
        var f = Promise.promisifyAll(imap.fetch(results, {
          bodies: ['HEADER.FIELDS (FROM TO CC BCC SUBJECT)', 'TEXT'],
          struct: true
        }));

        f.on('message', function(msg, seqno) {
          newMessage = {}

          msg.on('body', function(stream, info) {
            //When we get a message, append the header to header, text to body.
            stream.on('data', function(chunk) {
              if (info.which !== 'TEXT')
                newMessage.rawHeader += chunk.toString('utf8')
              else
                newMessage.body += chunk.toString('utf8')
            })

            //When stream is done, strip the unparsable characters at the beginning before parsing.
            //NOTE: I'm not actually sure what these unparseable characters actually are
            //but this all works kosher.
            stream.once('end', function() {
              if (info.which !== 'TEXT') {
                newMessage.rawHeader = newMessage.rawHeader.replace(/^undefined/, '')
                newMessage.header = Imap.parseHeader(newMessage.rawHeader)
              }
              if (newMessage.body)
                newMessage.body = newMessage.body.replace(/^undefined/, '')
            })
          })

          msg.once('attributes', function(attrs) {
            newMessage.attrs = attrs
          })

          msg.once('end', function() {
            messages[seqno - 1] = newMessage
          })

        });

        f.once('error', function(err) {
          throw err
        });

        return f.onceAsync('end').then(function() {
          console.log('Messages: ' + messages.length)
          // just resolve it, lets not worry about returning promises 3 or 4 callbacks deep
          resolve(messages)
        })
      }).catch(function(e) {
        // pass the failure to the promise
        reject(e)
      })
    }).catch(function(e) {
      reject(e)
    })
  })
}

module.exports = exports

@Bergi, what do you mean "no"? This seems a lot more straightforward and standard use of promises to me than `Promise.method ` — lyjackal, Aug 07 '15 at 14:51
"no" in the meaning of "no, don't do this, it's a bad practise"; please see the linked Q&A. But you're right, `Promise.method` is a bit superfluous here. — Bergi, Aug 07 '15 at 14:58
Fair enough, I didn't see the link. I do think the promise.method is the same anti pattern though — lyjackal, Aug 07 '15 at 15:17
I'm not saying your statement about `Promise.method` is wrong, or that it's an antipattern. The `Promise` constructor you've used in the presented code is. — Bergi, Aug 07 '15 at 15:25

dkran · Accepted Answer · 2015-08-07T20:11:36.643

0

As per my and @Bergi's conversation, I realized I just have to return the whole chain of promises. like so, and it works:

exports.parseMailbox = function(boxName){
  boxName = boxName || 'INBOX'
  return imap.openBoxAsync(boxName, false).then(function(inbox){
  var messages = [], newMessage = {};
    return imap.searchAsync(['ALL']).then(function(results){
      var f = Promise.promisifyAll(imap.fetch(results, {
        bodies: ['HEADER.FIELDS (FROM TO CC BCC SUBJECT)','TEXT'],
        struct: true
      }));

  f.on('message', function(msg, seqno) {
    newMessage = {}

    msg.on('body', function(stream, info) {
      //When we get a message, append the header to header, text to body.
      stream.on('data', function(chunk){
        if (info.which !== 'TEXT')
          newMessage.rawHeader += chunk.toString('utf8')
        else
          newMessage.body += chunk.toString('utf8')
      })

          //When stream is done, strip the unparsable characters at the beginning before parsing.
          //NOTE: I'm not actually sure what these unparseable characters actually are
          //but this all works kosher.
          stream.once('end', function() {
            if (info.which !== 'TEXT'){
              newMessage.rawHeader = newMessage.rawHeader.replace(/^undefined/, '')
              newMessage.header = Imap.parseHeader(newMessage.rawHeader)
            }
            if(newMessage.body)
              newMessage.body = newMessage.body.replace(/^undefined/, '')
          })
        })

        msg.once('attributes', function(attrs) {
          newMessage.attrs = attrs
        })

        msg.once('end', function() {
          messages[seqno-1] = newMessage
        })

      })

      f.onceAsync('error').catch(function(err){
        throw err
      })

      return f.onceAsync('end').then(function() {
        console.log('Messages: ' + messages.length)
        return messages
      })
    })
  }).catch(function(e){
    throw e
  })
}

module.exports = exports

Now, the promise returns the property like it should have.

edited Aug 07 '15 at 20:11

answered Aug 07 '15 at 13:23

dkran

284
2
4
16

As @lyjackal has said, the `Promise.method` wrapper is unnecessary here, you should consider omitting it. – Bergi Aug 07 '15 at 14:59
oh I see. As long as I'm returning a promise or throwing an error, the method wrapper is unnecessary. Edited. Looks like promise.method is only necessary if I don't return a promise and just return a value or something synchronous? – dkran Aug 07 '15 at 20:11
Exactly. It's especially suited for synchronously throwing functions. See also [the docs](http://bluebirdjs.com/docs/api/promise.method.html) – Bergi Aug 07 '15 at 21:48
@Bergi Why don't my errors throw this way though? I can't seem to get around that one. – dkran Aug 10 '15 at 19:23
Which errors do you mean? You're not throwing from the `parseMailBox` function. You throw from several promise callbacks, but those are getting caught and used to reject the resulting promise. – Bergi Aug 10 '15 at 20:35
but my resulting promise doesn't get rejected, it just returns an empty array. If I run this in a synchronous way and there are no messages, or the search term is invalid, fetch will throw a "no messages" error, or imap.search will throw a detailed error on why the search term is invalid. These errors don't seem to get tossed down here. I seem to just get a an empty response. – dkran Aug 11 '15 at 01:49
Yes, `f.onceAsync('error')` does create a promise that will get rejected. But nothing is happening with it! The error just gets swallowed. The `new Promise` you're constructing does not know anything about it. You rather should do `f.on('error', reject);` – Bergi Aug 11 '15 at 02:04
So generating a second promise actually exists separately from the second. even with your code, it still doesn't reject. (reject is not defined btw, but I tried a few other ways). Could this line be messing it up somehow: `var f = Promise.promisifyAll(imap.fetch({})` since I assign it to a variable? christ, node error handling was hard enough as it was ;) – dkran Aug 11 '15 at 20:35
Ooops, I somehow assumed you were in a `new Promise(function(resolve, reject) { … })` callback where `reject` is available. If you don't want to do that, you could also try `Promise.race([f.onceAsync('end'), f.onceAsync('error).then(function(e) { throw e; })]);` – Bergi Aug 11 '15 at 20:41

Promise not returning value

2 Answers2