forEach over array with .push into another array

Question

I'm working on a Node / Mongoose / Express app and I'm having problem getting an array to update while using forEach. I'm not 100% sure what I'm missing. Does this have something to do with synchronous vs asynchronous? I may also just have the return wrong.

router.route('/:slug/episodes')

.get(function(req, res) {
    var episodeArray = [];
    var episodeDetails = null;
    Show.findOne({ 'slug': req.params.slug }, function(err, show) {
        if (err) {
            res.send(err);
        }
        var episodes = show.episodes
        episodes.forEach(function(episodeID, index) {
            Episode.findById(episodeID, function(err, episode) {
                if (err) {
                    res.send(err);
                }
                episodeArray.push(episode);
            });
        });
        res.send(episodeArray)
    });
});

episodeArray isn't getting the episode added in and ends up just being full of null values.

Sounds like the classic async problem. See if this helps http://stackoverflow.com/questions/14220321/how-to-return-the-response-from-an-asynchronous-call — elclanrs, May 11 '15 at 23:43
what's the actual problem, though? I don't see anything that describes what you see it do, and what you expected it to do instead. — Mike 'Pomax' Kamermans, May 11 '15 at 23:54
Also you might have a performance issue. Why not get all the episodes from a slug at once? — davibq, May 11 '15 at 23:54
@Mike'Pomax'Kamermans the problem is that `episodeArray` isn't actually getting `episode` pushed into it. It's looping and coming up `null` — Eric Sandine, May 12 '15 at 00:21
No, I'm pretty sure it is. Check what the content is with a `console.log(episodeArray)` after your `episodeArray.push(episode)`. But then also notice that you have `res.send` getting called way too early, That `Episode.findById` is also async, so you'll need to wait for that to be done before `res.send` — Mike 'Pomax' Kamermans, May 12 '15 at 00:39
@EricSandine Can you post the function body of `findById`? Even if it's async, you're not pushing into the array until the callback for `findById` is being fired, so that shouldn't be a problem. It sounds instead like that 2nd argument( `episode`) is null or out of scope -- because it's pushing the right number of `null`s right? — Benny Schmidt, May 12 '15 at 00:54
oh yeah, and @Mike'Pomax'Kamermans is probably right about that `res.send(episodeArray)` at the bottom -- it shouldn't do that until all callbacks for `findById` have completed. You could check when the length of the array is sufficient and then fire the method that will call `res.send(episodeArray)`, for example — Benny Schmidt, May 12 '15 at 01:01

score 0 · Answer 1 · answered May 12 '15 at 01:13

You're not waiting for the asynchronous operations to finish before sending an array back to the client.

Try something like this:

    var togo = episodes.length;
    var error;
    episodes.forEach(function(episodeID, index) {
      Episode.findById(episodeID, function(err, episode) {
        if (err) {
          if (!error)
            error = err;
        } else
          episodeArray.push(episode);
        if (--togo === 0)
          res.send(error ? error : episodeArray);
      });
    });

Additionally you should really add a return; if you encounter an error during an async operation, in order to prevent the rest of the code from being executed.

score 0 · Accepted Answer · edited May 23 '17 at 12:21

Your code is suffering from a misunderstanding of how async works. First and foremost you should read the Felix Kling link that @elclanrs posted here. SO contributors tend to get tired of answering the same async question over and over. I haven't quite gotten that tired yet, so I'll bite for the purposes of explaining async, but I'll also suggest another option that might be a much better way to solve your problem.

The async solution: There are many ways to await an array of asynchronous operations to complete. async.queue is a popular choice. How it works is you push a set of pending operations into a queue and then tell that queue to wait until all results have been received, at which point you perform your res.send(). The code would look something like this:

var async = require('async');

Show.findOne({
    'slug': req.params.slug
}, function(err, show) {
        if (err) {
            res.send(err);
        }

        var episodeArray = [];
        var queue = async.queue(function(episodeID, callback) {
            Episode.findById(episodeID, function(err, episode) {
                if (err) {
                    throw err;
                }
                episodeArray.push(episode);
                callback();
            });
        });

        // Note that forEach is synchronous.
        // The tasks will be pushed to the queue before drain()
        episodes.forEach(function(episodeID, index) {
            queue.push(episodeId);
        });

        queue.drain = function() {
            res.send(episodeArray);
        };
    });

This is not the best way to solve your problem, this is only for the purpose of demonstrating how you could fix your existing code.

As long as your episodes array is not obscenely huge, then a far better way to query your episodes might be to use mongoDB's $in operator, like so:

Show.findOne({
    'slug': req.params.slug
}, function(err, show) {
        if (err) {
            res.send(err);
        }

        Episode.find({
            _id: {
                $in: show.episodes
            }
        }, function(err, episodes) {
                if (err) {
                    throw err;
                }
                res.send(episodes);
            });
    });

Edit:

If you want to go a bit deeper, I should also mention that mongoose supports promises, which you can use to make your code less nested and repetitive, like this:

Show.findOne({
    slug: req.params.slug
})
.then(function(show) {
    return Episode.find({
        _id: {
            $in: show.episodes
        }
    });
})
.then(function(episodes) {
    res.send(episodes);
})
// Any errors returned above will short circuit to the middleware next() here
.error(next);

I was still reading through the post to understand it better but thank you for the example. Also, using $in was perfect, I'm pretty new to deal with Mongo, I need there had to be a better way. Thanks! — Eric Sandine, May 12 '15 at 01:17
I don't want to be that nitpicky, but I wrote the linked the question/answer :P or are you referring to the comment? — Felix Kling, May 12 '15 at 01:30

forEach over array with .push into another array

2 Answers2