Warning: a promise was created in a handler but was not returned from it

[h4r1m4u]

Hi Andreas,

First of all, thank you very much for this wonderful library, it's really helpful. Much awesomeness.

Second, if I make the Mailchimp API request within the async.waterfall function using the callback style, I get the following warning:

Warning: a promise was created in a handler at /node_modules/mailchimp-api-v3/index.js:486:9 but was not returned from it, see http://goo.gl/rRqMUw

Here's a specific example (Express.js app):

async.waterfall([
  // Get user from the database 
  function(done) {
   db.getUser(userId, function(err, user) {
     done(err, user);
   });
  },
  // Subscribe them to Mailchimp
  function(user, done) { 
    mailchimp.post({
      path: '/lists/' + listId + '/members',
      body: {
        email_address: user.email,
        status: 'subscribed'
      }, function(err, response) {
        done(err);
    });
  }
], function(err) {
  if (err) {
    return next(err);
  }
  res.redirect('back');
});

If I make the same mailchimp.post request without using async waterfall, all is fine and I don't get the warning. The same happens with the other async control flow functions, like parallel.

I'm assuming it's because your library uses promises and it's generally a bad idea to mix async with them, but is there any simple way to resolve the issue?

I should note that the subscriber gets added to the list successfully, so the Mailchimp API call itself is working fine.

Many thanks in advance.

[thorning]

Thanks you, I'm happy the module sees some use :)'

I could not reproduce the warning. I have changed the return to null when using callbacks, which might solve the issue.

[h4r1m4u]

Hi Andreas,

Many thanks for the prompt response!

Strange that you could not reproduce the issue. If I have some time, I'll try to put together a bare-bones app that illustrates the problem. Just a thought: I get the warning only if the API call to Mailchimp is successful, so in the example code above this would be when the user gets successfully subscribed. If the user is already in the Mailchimp list and the API call returns an error as a result of it, the warning doesn't pop up.

Anyhow, I had a look at the update you made last night. Unfortunately, it doesn't resolve the issue, the warning still comes up. I spent some time today looking at the module's source code and promises in general and found two ways to solve the problem. I'll be using the request method as an example here, but the same would apply to the batch and batchWait methods.

1) Put the return null statement inside the then/catch statements when using callback, instead of outside of it, like so:

  if (done) {
    promise
      .then(function (result) {
        done(null, result);        
        return null;
      })
      .catch(function (err) {
        done(err);
        return null;
      });
  }

return promise;

2) Use the BlueBird .asCallback() method:

  if (done) {
    promise.asCallback(done);
  }

return promise;

Since the .asCallback() method is specifically designed to create APIs that both return promises and accept node-style callbacks, I think it'd be the preferred solution here. As a matter of fact, if the .asCallback() method is used, you don't need to check if the callback done function was supplied by the use and the entire request method can be simplified as such:

Mailchimp.prototype.request = function (options, done) {
  var mailchimp = this;
  // RETURN THE PROMISE RIGHT AWAY HERE 
  return new Promise(function(resolve, reject) {
    if (!options) {
      reject(new Error("No request options given"));
      return;
    }

    var path = formatPath(options.path, options.path_params);
    var method = options.method || 'get';
    var body = options.body || {};
    var params = options.params;
    var query = options.query;

    //Parems used to refer to query parameters, because of the mailchimp documentation.
    if (params) {
      console.warn('params is depricated, use query instead');
      if (!query) {
        query = params;
      }
    }

    if (!path || !_.isString(path)) {
      reject(new Error('No path given'))
      return;
    }

    request({
      method : method,
      url : mailchimp.__base_url + path,
      auth : {
        user : 'any',
        password : mailchimp.__api_key
      },
      json : body,
      qs : query,
      headers : {
        'User-Agent' : 'mailchimp-api-v3 : https://github.com/thorning/node-mailchimp'
      }
    }, function (err, response) {

      if (err) {
        reject(new Error(err))
        return;
      }

      if (response.statusCode < 200 || response.statusCode > 299) {
        reject(Object.assign(new Error(response.body.detail), response.body));
        return;
      }

      var result = response.body || {};
      result.statusCode = response.statusCode;

      resolve(result);
    })

  }).asCallback(done); // ADD ON THE .asCallback HERE TO SUPPORT BOTH PROMISES AND CALLBACKS
}

Both these solutions remove the warning when using the module together with the async library. I only tested the changes using the callback style only, but I don't think they should impact the promise style in any way.

Let me know what you think.

[thorning]

I will look closer at this when I have a little extra time. Is seems both options presented here boils down to returning a promise, even when a callback is supplied (moving the return null basically just removes the return to allow the promise to be returned).

I don't know if this is in-fact the best practice. I didn't do it because I don't think you ever want to mix the two, i.e. a callback and a .then on the same request.

[h4r1m4u]

Cool, thank you very much!

Is seems both options presented here boils down to returning a promise, even when a callback is supplied (moving the return null basically just removes the return to allow the promise to be returned).

Yeah, I mean the warning ultimately is about the promise being created but not returned which result in the runaway promise, so that makes sense.

As for the best practice, using the .asCallback() (or .nodeify()) methods seems to be a common recommendation:

http://stackoverflow.com/questions/23501146/creating-node-library-that-support-both-node-callbacks-and-promise-with-bluebird http://mono.software/2014/07/07/Creating-NodeJS-modules-with-both-promise-and-callback-API-support-using-Q/

I also found this post, which outlines how to implement both the callback and promise style, but doesn't use the .asCallback() method, helpful.

[thorning]

It seems bluebird have a specific option to supress this warning in cases like this. I have updated the module with this configuration. Let me know if there are still issues.

[h4r1m4u]

Andreas,

Thank you for following up and my apologies for the slow response; I've been traveling and largely away from the internet.

Personally I'm not sure if this is the best solution as it just suppresses the warning instead of resolving the cause. I think one of the approaches described in the links I posted above would be more appropriate.

Anyhow, I found a workaround with the .asCallback() method that works for me. When using the module in my code callback style, I just wrap the callback function in the .asCallback() method like so:

    mailchimp.post({
      path: '/lists/' + listId + '/members',
      body: {
        email_address: user.email,
        status: 'subscribed'
      }
    }).asCallback(function(err, response) {
        // Do something with response
    });

This fixes the warning and works well. Other modules, such as Knex.js, for example, also use this pattern. And since I use Knex with callbacks in my app extensively, it doesn't look out of place.

Thank you again for looking into the issue and your help. Much appreciated.