JavaScript: Accessing variable inside callback

Question

I'm trying to use the open-graph plugin in NodeJS to get a preview image for a vine. The OG result is correct, but I can't access result[i] from within the og callback - the variable is undefined. How can I access result[i] in the OG callback?

    Thing.find(function(err, result) {
        for (var i = 0; i < result.length; i++) {
            if (result[i].attachment) {
                if (result[i].attachment.embed_type == 'vine') {
                    og(result[i].attachment.embed_url, function(err, meta) {
                        result[i].attachment.preview_image = meta.image;
                        result[i].save();
                    });
                }
            }
        }
    });

possible duplicate of [JavaScript closure inside loops – simple practical example](http://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example) — Zirak, Apr 03 '15 at 23:39

adeneo · Accepted Answer · 2015-04-03T16:56:40.310

1

You need a closure, i keeps changing because og is async

Thing.find(function(err, result) {
    for (var i = 0; i < result.length; i++) {
        if ( result[i].attachment &&
             result[i].attachment.embed_type == 'vine') 
        {
            (function(res) {
                og(res.attachment.embed_url, function(err, meta) {
                    res.attachment.preview_image = meta.image;
                    res.save();
                });
            }(result[i]));
        }
    }
});

edited Apr 03 '15 at 16:56

answered Apr 03 '15 at 16:55

adeneo

312,895
29
395
388

This should be done outside the `for` loop for clarity. An IIFE is not the way to go here. – Sethen Apr 03 '15 at 16:56
In my opinion an IIFE is the way to go here, but any function will do – adeneo Apr 03 '15 at 16:57
It's not extendable and really just complicates things further. This is not correct and doesn't help the clarity and reusability of this code at all. – Sethen Apr 03 '15 at 16:58
Why is it not correct, and why would it complicate things ? – adeneo Apr 03 '15 at 16:59
What happens when I want to unit test that function separately? I should be allowed to do this easily by simply calling the function. You're also forcing other engineers to find the `for` loop this resides in if they want to make changes. The reason this should be in a function outside the `for` loop is the same reason why we use different classes to extend from. This isn't correct in my opinion and complicates matters further. While it may work, it's messy and foolish. – Sethen Apr 03 '15 at 17:00
Whatever floats your goat, but this is not a wrong way to do this, on the contrary it's a very common way to solve issues like this. I personally don't feel a need to have every little thing that needs a new scope moved out to new named functions in the global scope so the functions can be unit tested separately. – adeneo Apr 03 '15 at 17:04
I would say this isn't very common and you wouldn't see this in very many JavaScript libraries or frameworks because they know the issues this will cause. It's not really "whatever floats your goat", there's a right way to do this and a wrong way. Also, global scope wouldn't apply here if this is in a node module since they are scoped to their own module... So, not sure what you're even referring to here. That's only a problem if I am not wrapping something in an IIFE in the first place. This literally takes two seconds and gives you many, many benefits. – Sethen Apr 03 '15 at 17:06

score 0 · Answer 2 · edited May 23 '17 at 12:22

JS does not have block scope.

There are some ways to work around this problem, like creating a function, for example.

And, to make it more clear and easily testable, I would put all the result[n] logic in the other function as well.

var process = function(resultItem) {
   if (resultItem.attachment && if (resultItem.attachment.embed_type == 'vine')) {
     og(resultItem.attachment.embed_url, function(err, meta) {
       resultItem.attachment.preview_image = meta.image;
       resultItem.save();
     });
   }
};

Thing.find(function(err, result) {
  for (var i = 0; i < result.length; i++) {
     process(result[i]);
  }
});

JavaScript: Accessing variable inside callback

2 Answers2