Problem with JavaScript closure

Question

Hey guys, I am stuck at the following code. At first I'll describe the use-case: The function "addPreset" gets called with an instance of ColorGradient. When calling this.listController.addItem(...) a callback function named onSelect ist supplied, which gets called everytime the onSelect-event on the listController-item is triggered. What I wanted to do is wrapping the call to GLab.ColorSlider.applyColorGradient(...) into a new closure, so that the assigned value of addPreset's "cg" argument"* will be "caught" inside it. But it doesn't work.

PROBLEM: Now everytime addPreset is called, the value of cg (being passed with a call) will override all values that bad been assigned before. However, this.presetList holds always correct values (the ones I expected to be caught inside the closure-function. Even inserting an anonymous function for breaking the scope doesn't help.

Please help me. :-)

Thanks, so far

function addPreset(cg) {
    if (!(cg instanceof ColorGradient)) {
        throw new TypeError("PresetManager: Cannot add preset; invalid arguments received");
    }

    var newIndex = this.listController.addItem(cg.getName(), {
        onSelect: (function(cg2) {
            return function() {
                // addPreset's scope should now be broken
                GLab.ColorSlider.applyColorGradient(cg2);
                console.log(cg2);
            }
        })(cg)
    });

    this.presetList[newIndex] = cg;
}

@bobince: of course you can.

the code snippet above is part of PresetManager.js and the listController is an instance of the class ListWrapper.js

http://code.assembla.com/kpg/subversion/nodes/GradientLab/lib-js/PresetManager.js http://code.assembla.com/kpg/subversion/nodes/GradientLab/lib-js/ListWrapper.js

@Matt: cg is an instance of ColorGradient. A custom class of myself. Further more, it is assured, that always "valid" values are passed in as cg. (When you'd have a few minutes you can download the whole assembla repo as zip-archive. Unzip and test in FF > 3.5 with Firebug console enabled.)

Answer 1

Answer can be found in this question: http://stackoverflow.com/questions/643542/doesnt-javascript-support-closures-with-local-variables

Answer 2

Someone please correct me if I am wrong, as I am still fairly new to JavaScript closures and scope. But it would seem to me that the wrapping anonymous function you have is simply there to provide a proper scoped variable/closure for the function it is returning. Could this be simplified as such?

function addPreset(cg) {
            if (!(cg instanceof ColorGradient)) {
                throw new TypeError("PresetManager: Cannot add preset; invalid arguments received");
            }

            var closured = cg;
            var newIndex = this.listController.addItem(cg.getName(), {
                onSelect: function() {
                    // addPreset's scope should now be broken
                    GLab.ColorSlider.applyColorGradient(closured);
                    console.log(closured);
                }
            });

            this.presetList[newIndex] = cg;
        }

Answer 3

Just want to tell you, that I finally solved my problem by myself. It cost me almost 2 days (in the sparetime) to puzzling it out, but I think its worth that. At least my code remained elegant and I definitely got the whole thing with closures. Let's have a look:

My faulty code

Part 1 of 2:

function addPreset(cg) {
    if (!(cg instanceof ColorGradient)) {
        throw new TypeError("PresetManager: blablabla");
    }
    // calls the function in Part 2
    var newIndex = this.listController.addItem(cg.getName(), {
        onSelect: (function(cg2) {
            return function() {
                // addPreset's scope should now be broken
                GLab.ColorSlider.applyColorGradient(cg2);
                console.log(cg2);
            }
        })(cg)
    });

    this.presetList[newIndex] = cg;
}

Part 2 of 2:

// The method being called by this.listController.addItem(..)
function addItem(caption, args) {
    var _this = this,
        currIndex,
        id,
        newItem
        itemSelectCb = (!!args && typeof args.onSelect == "function") ?
                            args.onSelect :
                            undefined;

    currIndex = this.numOfItems;
    id = this.ITEM_ID_PREFIX + currIndex;
    newItem = this.$itemTemplate
        .clone()
        .text(caption)
        .attr("id", id)
        .bind("click", function(e) {
            e.stopPropagation();
            if (typeof itemSelectCb != "undefined") {
                itemSelectCb();
            }    
            _this._onSelect($(".ListWrapperItem").index(this));
        })
        .appendTo(this.$container);

    this.numOfItems = $("." + this.DEFAULT_ITEM_CLASS, this.$container).length;

    return currIndex;
}

The fixed code

The bug was in Part 2; when calld jQuery's bind-method for adding an click-event-listener I used an anonymous function (= new closure), but referenced itemSelectCb inside; so the anonymous function's scope stayed "connected" to the one of addItem. Everytime I called addItem, an other value were assigned toitemSelectCb what lead to the unknown sideeffect, that all references to itemSelect inside previously created anonymous functions are pointing to that value. What meant, that the last assigned value, had been used by all anonymous function.

To "break" the scope, all I had to do was to modify the lines of Part 2 where the event-handler for jQuery's bind was created. The fixed code looks then like this:

function addItem(caption, args) {
    var _this = this,
        currIndex,
        id,
        newItem
        itemSelectCb = (!!args && typeof args.onSelect == "function") ?
                            args.onSelect :
                            undefined;

    currIndex = this.numOfItems;
    id = this.ITEM_ID_PREFIX + currIndex;
    newItem = this.$itemTemplate
        .clone()
        .text(caption)
        .attr("id", id)
        .bind("click", (function(itemSelectCb) {    

            return function(e) {                    
                e.stopPropagation();
                if (typeof itemSelectCb != "undefined") {
                    itemSelectCb();
                }    
                _this._onSelect($(".ListWrapperItem").index(this));                    
            }

        })(itemSelectCb))
        .appendTo(this.$container);

    this.numOfItems = $("." + this.DEFAULT_ITEM_CLASS, this.$container).length;

    return currIndex;
}