Fetching and shortening URLs with API calls using Promises

Question

I have a node module that accesses the News API to fetch news articles and runs each article's URL through Google's URL shortener. It also has a caching mechanism implemented. This was previously written with callbacks, but I've rewritten it with Promises instead.

1. Is it also appropriate to catch errors and return my own Promise.reject in order to standardize the error format as I'm doing?

2. Should I just return the Promise and let the calling function catch the error?

In addition to a code review, I'd like to know if this is the proper way to use Promises as I've had no previous experience with Promises.

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

const NEWS_API_KEY = process.env.NEWS_API_KEY;
if (!NEWS_API_KEY) {
  throw new Error('No News API key specified. Make sure you have \
      NEWS_API_KEY in your environment variables.');
}

const URL_SHORTENER_API_KEY = process.env.URL_SHORTENER_API_KEY;
if (!URL_SHORTENER_API_KEY) {
  throw new Error('No URL Shortener API key specified. Make sure you have \
      URL_SHORTENER_API_KEY in your environment variables.');
}

/**
 * Base URL for the news API.
 * @type {string}
 */
const NEWS_API_BASE_URL = 'https://newsapi.org/v1/';


/**
 * Base URL for the URL Shortener API.
 * @type {type}
 */
const URL_SHORTENER_BASE_URL = 'https://www.googleapis.com/urlshortener/v1/url';

/**
 * The error string returned when an invalid source is queried.
 * @type {string}
 */
const BAD_SOURCE = 'sourceDoesntExist';

/**
 * Milliseconds in 10 minutes, the duration which results will be cached.
 * @type {number}
 */
const CACHE_KEEP_TIME = 600000;

const cache = {};

/**
 * This method sends a request to Google's URL Shortener API to get
 * a shortened URL and returns a Promise.
 * @param {string} url The URL to shorten.
 * @return {Promise}
 */
const shortenUrl = (url, callback) => {
  return request({
    uri: URL_SHORTENER_BASE_URL,
    method: 'POST',
    headers: {
      'Content-Type': 'application/json'
    },
    body: { longUrl: url },
    qs: { key: URL_SHORTENER_API_KEY },
    json: true
  }).then(data => {
    return Promise.resolve(data.id);
  }).catch(error => {
    return Promise.reject({
      message: 'URL Shortener API failure',
      error: error
    });
  });
};

/**
 * This method fetches article sources from the News API and passes it to
 * a callback. Any errors will be passed to the callback as well.
 * @param {Object} options Options for customizing the request
 * @return {Promise}
 */
const fetchSources = options => {
  return request({
    uri: NEWS_API_BASE_URL + 'sources',
    qs: options,
    json: true
  }).then(data => {
    return Promise.resolve(data.sources);
  }).catch(error => {
    return Promise.reject({
      message: 'News API source fetching failure',
      error: error
    });
  });
};

/**
 * This method fetches article data from the News API and returns a Promise.
 * @param {string} source The News API source to query.
 * @return {Promise}
 */
const fetchArticles = source => {
  /**
   * We first check if the query has been cached within the last 10
   * minutes. If it has, then we return the cached data. If not, we then
   * fetch new data from the News API.
   */
  const currentTime = Date.now();
  if (cache[source] && currentTime < cache[source].expires) {
    return Promise.resolve(cache[source].results);
  }
  /**
   * If the section being requested was not cached, then we need to fetch the
   * data from the News API.
   */
  return request({
    url: NEWS_API_BASE_URL + 'articles',
    qs: {
      'source': source,
      'apiKey': NEWS_API_KEY
    },
    json: true
  }).then(data => {
    /**
     * We shorten the URLs for each article.
     */
    return Promise.all(data.articles.map(article => {
      return shortenUrl(article.url).then(shortenedUrl => {
        article.url = shortenedUrl;
        return Promise.resolve(article);
      });
    }));
  }).then(data => {
    const results = data.sort((a, b) => {
      return a.title.localeCompare(b.title);
    });
    /**
     * We cache the result and then return it in a resolved Promise.
     */
    cache[source] = {
      results: results,
      expires: currentTime + CACHE_KEEP_TIME
    };
    return Promise.resolve(results);
  }).catch(error => {
    return Promise.reject({
      message: 'News API article fetching failure',
      error: error.constructor === Error ? error : error.error
    });
  });
};

module.exports = exports = {
  BAD_SOURCE: BAD_SOURCE,
  shortenUrl: shortenUrl,
  fetchSources: fetchSources,
  fetchArticles: fetchArticles
};

Answer 1

Is it also appropriate to catch errors and return my own Promise.reject in order to standardize the error format as I'm doing?

Yes, that's totally fine. Although you could create a custom error class, the best code is the one you never have to write.

The recommendation is to instantiate from the native error constructors instead of making your own custom constructor. That way, errors are all instances of native errors (you can do an instanceof easily) and have the usual properties of an error (message, stack trace, etc.). Other code will not need your code to know if it's an error.

Should I just return the Promise and let the calling function catch the error?

You can if your API is a thin wrapper/shorthand of some API. The error thrown by the internal logic would still make sense. But when your logic is more complex, simply forwarding an internal implementation's error might not make sense. You will need give the consumer a custom error so that it makes sense.

.then(data => {
  return Promise.resolve(data.id);
})

You can simply return data.id. Non-promise values returned by the callback become the resolved value of the promise returned by then. Apply this to the rest of the code doing this.

.catch(error => {
  return Promise.reject({
    message: 'URL Shortener API failure',
    error: error
  });
});

This is the case mentioned above. The error object may not make sense to the consumer. Returning your own custom error instance with that message might make more sense to the consumer. Leave the details of error to your code/logger/reporter.

return Promise.all(data.articles.map(article => {
  return shortenUrl(article.url).then(shortenedUrl => {
    article.url = shortenedUrl;
    return Promise.resolve(article);
  });
}));

Find out if this has a bulk counterpart. data.articles may be hundreds of urls, which means it will fire a hundred shortenUrls. While it's performant because it's async, lessening the number of requests is still ideal.

const results = data.sort((a, b) => {
  return a.title.localeCompare(b.title);
});

You can skip the brackets and return. An arrow function whose body is an expression returns the value of that expression implicitly.