2

I wrote some javascript to run through an array of "modules". For each module it makes an ajax call, gets the html for the module, and dumps the module into one of two columns on the page layout.

var modules = [
    { name: 'ExchangeStatus', title: 'Exchange Status', column: 'left' },
    { name: 'Grid', title: 'Contacts', column: 'right' }
    { name: 'Filters', title: 'Filters', column: 'left' }
],
modulesUrl = '/Modules/';

for (var count = 0; count < modules.length; count++) {
    var module = modules[count];

    // Get module HTML.
    $.ajax({
        url: modulesUrl + module.name,
        success: function (moduleHtml) {

            // Append module HTML to appropriate element.
            $('#' + module.column + 'Column').append(moduleHtml);

            // Custom event to trigger the module to load its data.
            var initializeModuleEvent = $.Event('initialize' + module.name);
            initializeModuleEvent.firstLoad = true;
            $(document).trigger(initializeModuleEvent);

        }
    });
}

Problem with this is that the for loop creates a race condition. In the ajax callback module.column is almost always the value of the last module because the for loop finishes long before the ajax callbacks return. How can I maintain the module variable so that each callback is dealing with the appropriate module? Currently all three modules end up in the left column because the last module in the loop is in the left column.

CatDadCode
  • 58,507
  • 61
  • 212
  • 318
  • possible duplicate of [Javascript closure inside loops - simple practical example](http://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example) – Felix Kling Jan 31 '13 at 23:38

5 Answers5

7

You could try creating an extra scope to pass module:

for (var count = 0; count < modules.length; count++) {
    var module = modules[count];

    (function(module) {
      $.ajax({
        ...
      })
    }(module));

Edit: If you want you can use forEach as well:

modules.forEach(function(module) {
 // 'module' will be available everywhere in this scope
});
elclanrs
  • 92,861
  • 21
  • 134
  • 171
  • Thanks so much! I feel dumb for not thinking of using a closure on my own but I really appreciate it :D – CatDadCode Jan 31 '13 at 23:11
  • This is not the only option tho, but the most obvious. You can use `forEach` and forget about closures if you want. – elclanrs Jan 31 '13 at 23:15
  • I didn't realize javascript had a `forEach`... I can't seem to get it to work. EDIT: Ohhh, function on the array itself. Nice. – CatDadCode Jan 31 '13 at 23:16
  • Good stuff. I like the `array.forEach()` method sooo much more. Creates a closure automatically and gets rid of all the ugly old school `for...loop` syntax. Thanks! – CatDadCode Jan 31 '13 at 23:21
  • @AlexFord: Glad it helped. Make sure to check http://kangax.github.com/es5-compat-table/ for browser compatibility info on all the new ES5 array methods, such as `forEach`. – elclanrs Jan 31 '13 at 23:28
  • It's not about creating a closure, after all, the success callback for the `$.ajax` call **is** a closure. It's about *executing* a new function and creating a new scope. It does not have to be a closure. – Felix Kling Jan 31 '13 at 23:37
  • Executing a new function (before the ajax call returns) **is** wrapping the variable in a new closure. The success callback is a closure, yes, but the `module` variable was defined in a higher scope. By wrapping both the ajax (or the callback definition) and the variable in the same function you create an enclosure for both together. – CatDadCode Jan 31 '13 at 23:44
  • 1
    I think the confusion here is that `closure == scope == function` most of the time in JS so the semantics my vary I guess... – elclanrs Jan 31 '13 at 23:45
  • @elclanrs With regard to browser compatibility, lucky for me this is an internal company application and we officially support Chrome and nothing else :D – CatDadCode Jan 31 '13 at 23:48
  • @Alex: Of course, technically every function is a closure in JS (because every function has the possibility to access variables defined in a higher scope). But again, the key point here is that the function is executed immediately and thus creating a new scope. You could also move the whole Ajax call in a function defined outside the `for` loop and it would still work. Being a closure is not a requirement the function has to meet in order to make this work. – Felix Kling Jan 31 '13 at 23:48
  • By moving the ajax call outside of the loop (into an external function I presume?) are you not indeed wrapping the ajax call in a closure and passing the `module` variable into it? `closure === scope` does it not? – CatDadCode Jan 31 '13 at 23:50
  • @Alex: If we define "closure" as a function which references free variables (i.e. variables defined outside the function and that are not global) then no. `$` would global and `modules` would be passed as argument. I can see that it is confusing and I'm not arguing that the function is not indeed a closure. I'm just saying that the statement "use a closure" is not correct because what you actually need is a function invocation, not a closure. *edit:* and I agree... it's all good ;) – Felix Kling Jan 31 '13 at 23:52
  • Hmmm, well this is about worthy of being moved to chat so I will just let it go lol – CatDadCode Jan 31 '13 at 23:53
2

Modify your success callback to this:

$.ajax({
    url: modulesUrl + module.name,
    success: (function (module) {
        return function (moduleHtml) {
            // Append module HTML to appropriate element.
            $('#' + module.column + 'Column').append(moduleHtml);

            // Custom event to trigger the module to load its data.
            var initializeModuleEvent = $.Event('initialize' + module.name);
            initializeModuleEvent.firstLoad = true;
            $(document).trigger(initializeModuleEvent);
        };
    }(module));
});

This will create a new scope for the appropriate module variable value.

There's no need to wrap the entire ajax call here.

Brian Ustas
  • 62,713
  • 3
  • 28
  • 21
2

instead of using the for loop use the jquery's each.. this will solve the problem

var modules = [
    { name: 'ExchangeStatus', title: 'Exchange Status', column: 'left' },
    { name: 'Grid', title: 'Contacts', column: 'right' }
    { name: 'Filters', title: 'Filters', column: 'left' }
],
modulesUrl = '/Modules/';

$(modules).each( function(index, elem)
{
    var module = elem;

    // Get module HTML.
    $.ajax({
        url: modulesUrl + module.name,
        success: function (moduleHtml) {

            // Append module HTML to appropriate element.
            $('#' + module.column + 'Column').append(moduleHtml);

            // Custom event to trigger the module to load its data.
            var initializeModuleEvent = $.Event('initialize' + module.name);
            initializeModuleEvent.firstLoad = true;
            $(document).trigger(initializeModuleEvent);

        }
    });
}
}
);
pranag
  • 5,432
  • 2
  • 17
  • 15
  • The only problem I've had with this in the past is that jQuery's `each` will also iterate over object properties. I had a nasty bug once where the server returned something wrong and jQuery was freezing the page trying to iterate over something it shouldn't. I'm probably paranoid now because of it, haha. – CatDadCode Jan 31 '13 at 23:42
1

That would help

for (var count = 0; count < modules.length; count++) {
    var module = modules[count];

    (function (module) {
        // Get module HTML.
        $.ajax({
            url: modulesUrl + module.name,
            success: function (moduleHtml) {

                // Append module HTML to appropriate element.
                $('#' + module.column + 'Column').append(moduleHtml);

                // Custom event to trigger the module to load its data.
                var initia`enter code here`lizeModuleEvent = $.Event('initialize' + module.name);
                initializeModuleEvent.firstLoad = true;
                $(document).trigger(initializeModuleEvent);

            }
        });
    })(module)
}
kidwon
  • 4,448
  • 5
  • 28
  • 45
0

Try this way and tell me if it works!

for (var n = 0; n < modules.length; n++) {
(function () {
    var i = n;
        var module = modules[i];

        // Get module HTML.
        $.ajax({
            url: modulesUrl + module.name,
            success: function (moduleHtml) {

                // Append module HTML to appropriate element.
                $('#' + module.column + 'Column').append(moduleHtml);

                // Custom event to trigger the module to load its data.
                var initializeModuleEvent = $.Event('initialize' + module.name);
                initializeModuleEvent.firstLoad = true;
                $(document).trigger(initializeModuleEvent);

            }
        });
    } ());
}