JavaScript closing problem

Question

JavaScript closing problem

Hey guys i am stuck with the following code. First I'll describe the use case: the "addPreset" function is called with a ColorGradient instance. Calling this.listController.addItem (...) invokes a callback function named onSelect ist, which is called every time the onSelect event fires on the listController element. I would like to make the call to GLab.ColorSlider.applyColorGradient (...) complete in a new closure so that the assigned value of the addPreset argument "cg" * will be "caught" inside it, but it won't work.

PROBLEM: now addPreset is called every time , the cg value (passed in with the call) overrides any previously assigned values. However, this.presetList always contains the correct values (the ones I expected to get inside the close function. Even inserting an anonymous function to break the scope does not help.

Please help me.:-)

Thanks, bye

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 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 listController is an instance of ListWrapper.js class

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 a ColorGradient example. Custom class for yourself. Moreover, I am sure that always "valid" values are passed as cg. (If you have a few minutes, you can download the entire repo builder as a zip archive. Expand and test in FF> 3.5 with the Firebug console enabled.)

+2

javascript closures

kishkash 23 Sep 09 at 13:42

source to share

3 answers

simon · Answer 1 · 2009-09-23T14:16:17+0000

The answer can be found in this question: Does JavaScript support local variables?

Matt · Answer 2 · 2009-09-23T15:19:28+0000

Someone please correct me if I'm wrong as I'm still pretty new to JavaScript closure and scope. But it looks to me like you have an anonymous wrapping function to provide a suitable variable / closure for the function it returns. 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 scope should now be broken
                    GLab.ColorSlider.applyColorGradient(closured);
                    console.log(closured);
                }
            });

            this.presetList[newIndex] = cg;
        }

kishkash · Answer 3 · 2009-09-24T09:06:02+0000

Just want to tell you that I finally solved my problem myself. It took me almost 2 days (in my spare time) to puzzle him, but I think it's worth it. At least my code remained elegant and I definitely got everything with closures. Let's get a look:

My wrong 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 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;
}

Fixed code

The error was in Part 2; when calld jQuery bind-method to add click-event listener I used anonymous function (= new closure) but referenced itemSelectCb inside; so the anonymous function scope remained "connected" to one of the addItems . Each time I called addItem, a different value was assigned to toitemSelectCb, with an unknown side effect that all references to itemSelect inside the previously created anonymous functions point to that value. Which meant the last assigned value was used by all anonymous functions.

To "split" the scope, all I had to do was change the lines Part 2 , where the event handler for jQuery bind was created. The fixed code looks 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;
}

JavaScript closing problem

My wrong code

Fixed code

More articles: