0

I am trying to work through JS Promises in node.js and don't get the solution for passing promises between different function.

The task

For a main logic, I need to get a json object of items from a REST API. The API handling itself is located in a api.js file.

The request to the API inthere is made through the request-promise module. I have a private makeRequest function and public helper functions, like API.getItems().

The main logic in index.js needs to wait for the API function until it can be executed.

Questions

  1. The promise passing kind of works, but I am not sure if this is more than a coincidence. Is it correct to return a Promise which returns the responses in makeRequest?
  2. Do I really need all the promises to make the main logic work only after waiting for the items to be setup? Is there a simpler way?
  3. I still need to figure out, how to best handle errors from a) the makeRequest and b) the getItems functions. What's the best practice with Promises therefor? Passing Error objects?

Here is the Code that I came up with right now:

// index.js

var API    = require('./lib/api');

var items;

function mainLogic() {

  if (items instanceof Error) {
    console.log("No items present. Stopping main logic.");
    return;
  }

  // ... do something with items
}


API.getItems().then(function (response) {
  if (response) {
    console.log(response);
    items = response;
    mainLogic();
  }
}, function (err) {
  console.log(err);
});

api.js

// ./lib/api.js

var request = require('request-promise');

// constructor
var API = function () {
  var api = this;

  api.endpoint = "https://api.example.com/v1";
  //...
};

API.prototype.getItems = function () {
  var api      = this;
  var endpoint = '/items';


  return new Promise(function (resolve, reject) {
    var request = makeRequest(api, endpoint).then(function (response) {
      if (200 === response.statusCode) {
        resolve(response.body.items);
      }
    }, function (err) {
      reject(false);
    });
  });
};

function makeRequest(api, endpoint) {
  var url     = api.endpoint + endpoint;
  var options = {
    method: 'GET',
    uri: url,
    body: {},
    headers: {},
    simple: false,
    resolveWithFullResponse: true,
    json: true
  };

  return request(options)
    .then(function (response) {
      console.log(response.body);
      return response;
    })
    .catch(function (err) {
      return Error(err);
    });
}

module.exports = new API();

Some more background: At first I started to make API request with the request module, that works with callbacks. Since these were called async, the items never made it to the main logic and I used to handle it with Promises.

Cino
  • 25
  • 1
  • 8
  • 2
    You're creating too many promises here, and one wrapping an existing promise in `new Promise` which [is an anti-pattern](http://stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it). You can just `return request(options)` from `makeRequest`, `return makeRequest(api, endpoint)` from `getItems` and finally, it looks like `mainLogic()` should call `getItems` rather than the other way around – CodingIntrigue Jan 08 '16 at 10:46

1 Answers1

4

You are missing two things here:

  1. That you can chain promises directly and
  2. the way promise error handling works.

You can change the return statement in makeRequest() to:

return request(options);

Since makeRequest() returns a promise, you can reuse it in getItems() and you don't have to create a new promise explicitly. The .then() function already does this for you:

return makeRequest(api, endpoint)
    .then(function (response) {
        if (200 === response.statusCode) {
            return response.body.items;
        }
        else {
            // throw an exception or call Promise.reject() with a proper error
        }
    });

If the promise returned by makeRequest() was rejected and you don't handle rejection -- like in the above code --, the promise returned by .then() will also be rejected. You can compare the behaviour to exceptions. If you don't catch one, it bubbles up the callstack.

Finally, in index.js you should use getItems() like this:

API.getItems().then(function (response) {
  // Here you are sure that everything worked. No additional checks required.
  // Whatever you want to do with the response, do it here.
  // Don't assign response to another variable outside of this scope.
  // If processing the response is complex, rather pass it to another 
  // function directly.
}, function (err) {
  // handle the error
});

I recommend this blog post to better understand the concept of promises:

https://blog.domenic.me/youre-missing-the-point-of-promises/

lex82
  • 11,173
  • 2
  • 44
  • 69
  • Thank you for this great reply. That really helped me to understand Promises much much better. Based on your answer, it all works fine now, however, errors are not passed properly from `makeRequest` up to the `API.getItems().then` function. I'll work on that now. – Cino Jan 08 '16 at 17:05