83

I am using Typeahead by twitter. I am running into this warning from Intellij. This is causing the "window.location.href" for each link to be the last item in my list of items.

How can I fix my code?

Below is my code:

AutoSuggest.prototype.config = function () {
    var me = this;
    var comp, options;
    var gotoUrl = "/{0}/{1}";
    var imgurl = '<img src="/icon/{0}.gif"/>';
    var target;

    for (var i = 0; i < me.targets.length; i++) {
        target = me.targets[i];
        if ($("#" + target.inputId).length != 0) {
            options = {
                source: function (query, process) { // where to get the data
                    process(me.results);
                },

                // set max results to display
                items: 10,

                matcher: function (item) { // how to make sure the result select is correct/matching
                    // we check the query against the ticker then the company name
                    comp = me.map[item];
                    var symbol = comp.s.toLowerCase();
                    return (this.query.trim().toLowerCase() == symbol.substring(0, 1) ||
                        comp.c.toLowerCase().indexOf(this.query.trim().toLowerCase()) != -1);
                },

                highlighter: function (item) { // how to show the data
                    comp = me.map[item];
                    if (typeof comp === 'undefined') {
                        return "<span>No Match Found.</span>";
                    }

                    if (comp.t == 0) {
                        imgurl = comp.v;
                    } else if (comp.t == -1) {
                        imgurl = me.format(imgurl, "empty");
                    } else {
                        imgurl = me.format(imgurl, comp.t);
                    }

                    return "\n<span id='compVenue'>" + imgurl + "</span>" +
                        "\n<span id='compSymbol'><b>" + comp.s + "</b></span>" +
                        "\n<span id='compName'>" + comp.c + "</span>";
                },

                sorter: function (items) { // sort our results
                    if (items.length == 0) {
                        items.push(Object());
                    }

                    return items;
                },
// the problem starts here when i start using target inside the functions
                updater: function (item) { // what to do when item is selected
                    comp = me.map[item];
                    if (typeof comp === 'undefined') {
                        return this.query;
                    }

                    window.location.href = me.format(gotoUrl, comp.s, target.destination);

                    return item;
                }
            };

            $("#" + target.inputId).typeahead(options);

            // lastly, set up the functions for the buttons
            $("#" + target.buttonId).click(function () {
                window.location.href = me.format(gotoUrl, $("#" + target.inputId).val(), target.destination);
            });
        }
    }
};

With @cdhowie's help, some more code: i will update the updater and also the href for the click()

updater: (function (inner_target) { // what to do when item is selected
    return function (item) {
        comp = me.map[item];
        if (typeof comp === 'undefined') {
            return this.query;
        }

        window.location.href = me.format(gotoUrl, comp.s, inner_target.destination);
        return item;
}}(target))};
iCodeLikeImDrunk
  • 17,085
  • 35
  • 108
  • 169
  • 1
    Possible duplicate of [How to avoid access mutable variable from closure](https://stackoverflow.com/questions/13813463/how-to-avoid-access-mutable-variable-from-closure) – Boghyon Hoffmann Jun 27 '17 at 10:44

5 Answers5

157

I liked the paragraph Closures Inside Loops from Javascript Garden

It explains three ways of doing it.

The wrong way of using a closure inside a loop

for(var i = 0; i < 10; i++) {
    setTimeout(function() {
        console.log(i);  
    }, 1000);
}

Solution 1 with anonymous wrapper

for(var i = 0; i < 10; i++) {
    (function(e) {
        setTimeout(function() {
            console.log(e);  
        }, 1000);
    })(i);
}

Solution 2 - returning a function from a closure

for(var i = 0; i < 10; i++) {
    setTimeout((function(e) {
        return function() {
            console.log(e);
        }
    })(i), 1000)
}

Solution 3, my favorite, where I think I finally understood bind - yaay! bind FTW!

for(var i = 0; i < 10; i++) {
    setTimeout(console.log.bind(console, i), 1000);
}

I highly recommend Javascript garden - it showed me this and many more Javascript quirks (and made me like JS even more).

p.s. if your brain didn't melt you haven't had enough Javascript that day.

Maciej Jankowski
  • 2,794
  • 3
  • 26
  • 33
64

You need to nest two functions here, creating a new closure that captures the value of the variable (instead of the variable itself) at the moment the closure is created. You can do this using arguments to an immediately-invoked outer function. Replace this expression:

function (item) { // what to do when item is selected
    comp = me.map[item];
    if (typeof comp === 'undefined') {
        return this.query;
    }

    window.location.href = me.format(gotoUrl, comp.s, target.destination);

    return item;
}

With this:

(function (inner_target) {
    return function (item) { // what to do when item is selected
        comp = me.map[item];
        if (typeof comp === 'undefined') {
            return this.query;
        }

        window.location.href = me.format(gotoUrl, comp.s, inner_target.destination);

        return item;
    }
}(target))

Note that we pass target into the outer function, which becomes the argument inner_target, effectively capturing the value of target at the moment the outer function is called. The outer function returns an inner function, which uses inner_target instead of target, and inner_target will not change.

(Note that you can rename inner_target to target and you will be okay -- the closest target will be used, which would be the function parameter. However, having two variables with the same name in such a tight scope could be very confusing and so I have named them differently in my example so that you can see what's going on.)

cdhowie
  • 158,093
  • 24
  • 286
  • 300
  • what about the second part? where i set the href for the button? this way of doing things seem super weird to me =/ – iCodeLikeImDrunk May 23 '13 at 22:07
  • 3
    @yaojiang Apply the same technique there. If you need help let me know and we can move to chat. But first try to understand what's happening in my example, and try to do it yourself. (The concept will stick better if you do it yourself!) – cdhowie May 23 '13 at 22:08
  • 2
    It's important to note that anonymous functions close around outer variables *by reference*. If you want to close around them by value then you need to introduce a new variable that has the value you want to capture, and will not change -- which, due to the fact that JavaScript does not have block scope, is most easily implemented using function parameters. – cdhowie May 23 '13 at 22:10
  • 2
    @yaojiang This is not a technique unique to JavaScript, rather it is widely used in functional languages that have function scope. It seems strange but is also incredibly powerful. Give the concepts (particularly closure) some time to sink in and you will be a better programmer for it. (I did not realize that C# anonymous methods were closures until after doing an extensive stint with JavaScript, and when I came back to C# realized that I had been missing out on an incredibly powerful tool.) – cdhowie May 23 '13 at 22:21
  • This has gave me a boost in learning more advance javascript including closures, thanks for this snippet!!! – T Shoats Aug 14 '16 at 20:42
12

In ecmascript 6 we have new opportunities.

The let statement declares a block scope local variable, optionally initializing it to a value. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let

Bogdan Ruzhitskiy
  • 1,167
  • 1
  • 12
  • 21
2

Since the only scoping that JavaScript has is function scope, you can simply move the closure to an external function, outside of the scope you're in.

Oded Breiner
  • 28,523
  • 10
  • 105
  • 71
0

Just to clarify on @BogdanRuzhitskiy answer (as I couldn't figure out how to add the code in a comment), the idea with using let is to create a local variable inside the for block:

for(var i = 0; i < 10; i++) {
    let captureI = i;
    setTimeout(function() {
       console.log(captureI);  
    }, 1000);
}

This will work in pretty much any modern browser except IE11.

ivanhoe
  • 4,593
  • 1
  • 23
  • 22