2
\$\begingroup\$

I'm using HapiJS to create a little backend for my side-project. In grabbing RSS feeds, I want to loop through my list of website URLs, grab the correct RSS links in the <head>, then loop through the RSS feeds, pulling out the articles, to return an aggregated JSON response (via the reply() call in HapiJS), to have all these sites articles in one response for my API.

I'm fumbling with a couple of things:

  1. The async nature of Node/JavaScript and doing what I want here in the most efficient/canonical manner
  2. And getting my data within the innermost loop to the outer one to return (scope).

This code console.logs all the article data correctly, but I'm unsure how to properly place the callback() to get these arrays of articles from each site, into one larger array that I will eventually return. This is the one controller in my HapiJS backend that is handling this (suggestions on how to break this up better welcome as well).

var feed = require('feed-read');
var request = require('request');
var _ = require('lodash');
var finder  = require('find-rss');
var async = require('async');

module.exports = blogs;

function blogs () {
  var articles;
}

blogs.getAll = function (req, reply) {
  //TODO: Handle facebook tech blog which probably requires an accept header properly set
  //TODO: Store all proper web/rss feeds somewhere
  //TODO: periodically loop through them and get new articles, and store them
  //TODO: Clean up API instead of one monolithic endpoint

    //iterate over all sites
    var sites = [
      'http://eng.joingrouper.com',
      'http://eng.rightscale.com',
      'https://engineering.linkedin.com/blog',
      'http://blog.stackoverflow.com/',
      'http://googledevelopers.blogspot.com/',
      'http://yahooeng.tumblr.com/',
      'http://code.flickr.net/',
      'http://engineering.pinterest.com/',
      'https://blog.twitter.com/engineering',
      'http://blog.42floors.com/',
      'http://engineering.flipboard.com/',
      'http://codeascraft.com/',
      'http://engineering.foursquare.com/',
      'http://instagram-engineering.tumblr.com/',
      'https://code.facebook.com/posts/',
      'https://labs.spotify.com/',
      'http://engineering.voxer.com/',
      'http://www.ebaytechblog.com/',
      'http://tech.gilt.com/',
      'http://engineering.heroku.com/',
      'http://www.thumbtack.com/engineering/',
      'http://engineering.silk.co/',
      'https://www.paypal-engineering.com/',
      'http://code.zynga.com/',
      'http://blog.risingstack.com/',
      'https://tech.blog.box.com/',
      'https://engineering.groupon.com/',
      'http://dev.hubspot.com/blog',
      'http://code.hootsuite.com/',
      'http://www.buzzfeed.com/techblog',
      'https://medium.com/medium-eng',
      'http://code.mixpanel.com'
    ];
    var feeds = [];
    var articles = [];
    var href;

    //var results = blogs.getAllFeeds(sites, articles);
    // 1st para in async.each() is the array of items
    async.each(sites,
      // 2nd param is the function that each item is passed to
      function(site, callback){
        // Call an asynchronous function, often a save() to DB
          console.log('start first site..');
          // Async call is done, alert via callback
              //Discover feed
              finder(site, function(error, response, body){
                if (error) {
                  return error;
                }
                  if(response[0]){
                    href = response[0]['href'];
                  }
                  //TODO: handle these conditions better
                  if(href === '/atom.xml' ||
                     href === '/rss/' ||
                     href === '/feed.xml'){
                       //Make full urls out of those sites that reference their feed relatively
                       href = site + href;
                  }
                  //Parse feed for articles
                  feed(href, function(err, articles) {
                     if (err) {
                         console.log("SITE: "  + site);
                         console.log("Error: " + err);
                         console.log("---------------------------------------------");
                     } else {
                      var _articles = [];
                      _.forEach(articles, function(article) {
                        //console.log(article['title']);
                        _articles.push(article['title'], article['link']);
                      });
                      console.log(_articles);
                     }
                  });
        });
        blogs.checkDone(sites);
      },
      // 3rd param is the function to call when everything's done
      function(err, _articles, articles){
        if(err){
          console.log(err);
        }
        console.log('done');
        reply('ap');
        // All tasks are done now
        //checkDone(sites);
      }
    );
    if(!articles){
      reply(articles);
    }
};

blogs.checkDone = function(object) {
  _.after(object.length, function(){
      console.log('done here');
  });
};
\$\endgroup\$
0

1 Answer 1

1
\$\begingroup\$

So I've done a rather in depth review of your code which I'll post below. It's fully commented so it should hopefully make sense. I haven't tested it but I should not have changed any functionality.

There were a few things I had to make assumptions for because of missing information. For example, what happens if the response[0].href (in my code, first) is not /rss/, /feed.xml or /atom.xml? In my code, I made it such that in these cases an empty array is passed - In the end result this array is flattened so it doesn't show.

The main thing I noticed is that you don't quite grasp callbacks and limitation of scope just yet. For example you have a href variable exposed to the entire of getAll when really that variable only needs to exist when you call feed inside of a nested scope.

In addition, you do this strange thing where you call blogs.checkDone(sites) inside of a function that can only be called whilst we are checking sites - and you don't really do anything except log it, when you could really avoid the blogs.checkDone(sites) function altogther by listening to the callback of getAll. With that in mind, here is the revised code I put together for you. It is about 20 lines shorter (if you factor in the urls I removed), doesn't have nearly as much code duplication and hopefully makes more sense to you than your original code would make if you looked at it in 6 months time.

The main thing i changed was that I changed async.each to async.map; async.each is only really for executing a function multiple times in a row and discarding the result. This fine but in your case you end up having to create scope variables that aren't necessary. Instead, async.each allows the receiver of the callback from your function to simply catch the results as their second parameter.

var url = require('url')
  , _ = require('lodash')
  , feed = require('feed-read')
  , async = require('async')
  , finder = require('find-rss');

// First move the site array outside of the blog function, 
// we should pass this into the module.
var sites = [
  'http://eng.joingrouper.com',
  'http://eng.rightscale.com',
  'https://engineering.linkedin.com/blog',
  ....
];

// Blog.js
// NB You may want to turn this into a newable type rather than
// using functional notation
function Blog(sites) {
  // Your get all function.
  // I have removed your 'req' argument as it is not used, and I have
  // changed 'reply' to 'done' for semantics.
  // done will be invoked when getAll is completed.
  // done should have the signature (error, results)
  getAll = function() {
    // Use map, not each - each is simply for performing a function x times.
    // map is used when you expect a value; it's callback then allows you
    // to catch a result as well as an error
    async.map(sites, 
      function(site, callback) {
        finder(site, function(error, feeds, body) {
          // If error is an error, then invoke the callback
          // with the error. this is propogated to async's 3rd argument.
          if(_.isError(error)) return callback(error);
          // What happens if response[0] is null/undefined? Your code will break.
          // As this is a collection result I'll reply with an empty array.
          if(_.isEmpty(feeds)) return callback(null, []]);
          // What happens if href is not /atom.xml or /rss/ or /feed.xml?
          var first = _(_.first(feeds).href);
          if(!first.includes('/atom.xml') || !first.includes('/rss/') || !first.includes('/feed.xml'))
            return callback(null, []);
          var fullUrl = url.resolve(site, first);
          feed(fullUrl, function(error, articles) {
            if(_.isError(error)) return callback(error);
            // No need to push to intermediate array.. we can just map the array.
            var results = _.map(articles, function(article) {
              // In your original code, you did .push(title,link).
              // This would push title and link to separate elements.
              // I assume you meant to push title and link to a single object.
              return { title: article.title, link: article.link };
            });
            // Invoke callback with the completed results
            callback(null, results);
          });
        });
      },
      function(err, results) {
        // Flatten nested arrays to single dimensional array
        done(err, _.flatten(results));
      });
  };

  // No need for a 'check done' function, and that does not make sense
  // anyway.

  return {
    getAll: getAll
  };
}
module.exports = Blog;

The usage now looks a bit like this:

var blog = require('./blog');
blog(['http://code.mixpanel.com', ....]).getAll(function(error, results) {
  if(_.isError(error)) throw error;
  // do something with results
});

If you have any further questions please feel free to ask.

\$\endgroup\$
6
  • \$\begingroup\$ Thanks Dan! Perfect. This got me up and running. Couple of things: (1) I had to add require('url') for your 'url.resolve' usage (2) I had to add (req,reply) back to the siqnature, and change 'done' back to 'reply', because that's what HapiJS expects. Then all was right. To answer some of your other Q's: (1) Yes, async.map is what I wanted, I didn't understand how .each worked. (2) 'What happens if response[0] is null/undefined?' Yup, I'll handle this better, as well as timeouts. \$\endgroup\$ Mar 15, 2015 at 9:50
  • \$\begingroup\$ (3) The intention of the inflexible '/atom.xml' checking is a matter of handling relative urls in the head of the HTML page for their RSS feeds. Some are full urls, some are relative. So I think I'll change it to something that says 'is full url' if not, then concatenate the '/whatevs' with 'example.com'. I'll have to look if the url lib in Nodejs can do this easily. I don't think url.resolve will do what I want. (4) Yes I wanted to push title, url as single object, thank you! \$\endgroup\$ Mar 15, 2015 at 9:57
  • \$\begingroup\$ Additional Q's: // NB You may want to turn this into a newable type rather than // using functional notation Can you explain why newable vs func, and when you'd want one over the other? \$\endgroup\$ Mar 15, 2015 at 9:59
  • \$\begingroup\$ Additional Q's: I obviously haven't quite wrapped my head around plain callbacks in JS yet. (I've used promises before to get around them.) When doing callbacks, how do you know what to pass? callback(null, results); What's null here for? Any solid, callback JS tutorials you know about you could point me to? \$\endgroup\$ Mar 15, 2015 at 10:01
  • \$\begingroup\$ Additional Q's: Using lodash, what's the best way to sort the flattened array by published? { title: 'Internship stories', link: 'https://code.mixpanel.com/2011/11/15/internship-stories/', published: Tue Nov 15 2011 18:54:15 GMT-0500 (EST), author: 'Suhail Doshi', site: 'https://code.mixpanel.com' }, { title: 'Why We Moved Off The Cloud', link: 'https://code.mixpanel.com/2011/10/27/why-we-moved-off-the-cloud/', published: Thu Oct 27 2011 19:36:08 GMT-0400 (EDT), author: 'mxpnl', site: 'https://code.mixpanel.com' }, _.sortBy() ? \$\endgroup\$ Mar 15, 2015 at 10:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.