jquery defered returning too early

Question

I have the following code:

$.when(multipleQueries(stateQueries, rowData))
    .then(function (data) {
        //do stuff with data - removed for brevity

The multipleQueries function is below:

function multipleQueries(queriesToExecute, rowData) {
    var allQueriesMapped = $.Deferred();

    // If a single query has been provided, convert it into an array
    if (Array.isArray(queriesToExecute) === false) {
        queriesToExecute = [].concat(queriesToExecute);
    }

    // Create a function for each region to run the query.
    $.when.apply($, $.map(queriesToExecute, function (query) {

        // Execute the query in the region
        return $.when(executeQuery(query.InstanceInfo, query.Query)).then(function (data) {
            var isDataMapped = $.Deferred();
            var mappedData = [];
            // Perform some data transformation
            $.when.apply($, $.map(data.results, function (value) {
                var properties = value.succinctProperties;
                 //code removed for brevity
                return $.when(mapData(properties)).then(function (mappedRow) {
                    if (mappedRow) {
                        mappedData.push(mappedRow);
                    }
                });
            })).then(function () {
                isDataMapped.resolve({
                    results: mappedData,
                    numItems: mappedData.length
                });
            });
            return isDataMapped.promise();
        }).then(function (data) {
            debugger;
            allQueriesMapped.resolve(data);
        });
    }));

    return allQueriesMapped.promise();
}

The issue I am having is that I am passing in say 5 queries to excute to the multipleQueries function but it is hitting the debugger line after running the first query - this is then resolving the allQueriesMapped deferred and then it returns to the do stuff with data where it was called from but because I dont have all the data from the 5 queires I passed in I am not seeing the expected behaviour - is there something missing with how I have set up these promises?

Note - I tried changing the .then before the debugger to .done but getting same behavior and also tried to change the calling code to .done as below but getting the same as well.

$.when(multipleQueries(stateQueries, rowData))
    .done(function (data) {
        //do stuff with data - removed for brevity

** Update - the execute query function is below

function executeQuery(instanceInfo, query) {
    return $.ajax({
        url: instanceInfo.Url,
        type: 'GET',
        data: {
            q: query,
            succinct: true
        },
        processData: true
    });
}

Side note: `queriesToExecute = [].concat(queriesToExecute);` => `queriesToExecute = [queriesToExecute];`. No need to create and throw away a temporary array and make a function call just to wrap something in an array. — T.J. Crowder, May 01 '18 at 10:29
It looks like it's hitting your debugger line in the first `then` because that `then` is attached to each of the `$.when` within the loop. ie `when(A,B,C) { when(a).then(a), when(b).then(b), when(c).then(c) }` — freedomn-m, May 01 '18 at 10:31
@T.J.Crowder executeQuery is another function - it calls off to an API to do the query — Ctrl_Alt_Defeat, May 01 '18 at 10:39
@T.J.Crowder - updated the question to include that function — Ctrl_Alt_Defeat, May 01 '18 at 10:44
Note you can return `$.when` instead of creating new `$.Deferred();`. You do that in 2 places. Is an ant-pattern since `$.when` already returns a promise — charlietfl, May 01 '18 at 10:45
You've removed code for brevity and still this function is hard to comprehend, looks like a maintenance nightmare tbh! Are you doing async stuff in all these $whens? — Dominic, May 01 '18 at 10:45
Related to what @charlietfl said, when you already have a promise (as from `$.ajax`), you don't need to use `$.when` on it, and doing so is both hard to read and inefficient. — T.J. Crowder, May 01 '18 at 10:48
The `then()` with `debugger` is on wrong level. It is inside the first `map()`. Move it out of that loop — charlietfl, May 01 '18 at 10:48
@charlietfl that's what I said as well :) (just worded it differently) — freedomn-m, May 01 '18 at 10:49
Avoid the [deferred antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! — Bergi, May 01 '18 at 10:53
@freedomn-m - could you include an answer based on your observation — Ctrl_Alt_Defeat, May 01 '18 at 10:55
@Bergi - thanks for link - ill take a read - I didnt write the code - just tasked with making a bug fix work so not looking to refactor the whole thing - but will take a read of your link for my own information — Ctrl_Alt_Defeat, May 01 '18 at 10:56
@T.J.Crowder - could you include a possible answer based on your observations - also including the line about not needing to create and throw away an array - ill make that change as well — Ctrl_Alt_Defeat, May 01 '18 at 10:58
I didn't want to steal @freedomn-m's / charlietfl's thunder. :-) They're the ones who spotted the primary issue. — T.J. Crowder, May 01 '18 at 11:01
@T.J.Crowder - no problem - hope they can elaborate with an answer as I am still not sure on the fix based on the comments - I will include you improvement on the unnecessary creation of array - Thanks — Ctrl_Alt_Defeat, May 01 '18 at 11:05
@charlietfl - could you include an answer based on the issue you have spotted — Ctrl_Alt_Defeat, May 01 '18 at 11:14
@T.J.Crowder no worries - from all the nested when/then/when/when/maybe then/ it wasn't clear if that *was* the answer (just looked wrong). — freedomn-m, May 01 '18 at 11:19
@freedomn-m - moving the then with debugger up a level gave same result not working — Ctrl_Alt_Defeat, May 01 '18 at 12:34

T.J. Crowder · Answer 1 · 2018-05-01T11:24:19.173

As freedomn-m and charlietfl pointed out, this then is in the wrong place (see *** comment):

function multipleQueries(queriesToExecute, rowData) {
    var allQueriesMapped = $.Deferred();

    // If a single query has been provided, convert it into an array
    if (Array.isArray(queriesToExecute) === false) {
        queriesToExecute = [].concat(queriesToExecute);
    }

    // Create a function for each region to run the query.
    $.when.apply($, $.map(queriesToExecute, function(query) {

        // Execute the query in the region
        return $.when(executeQuery(query.InstanceInfo, query.Query)).then(function(data) {
            var isDataMapped = $.Deferred();
            var mappedData = [];
            // Perform some data transformation
            $.when.apply($, $.map(data.results, function(value) {
                var properties = value.succinctProperties;
                //code removed for brevity
                return $.when(mapData(properties)).then(function(mappedRow) {
                    if (mappedRow) {
                        mappedData.push(mappedRow);
                    }
                });
            })).then(function() {
                isDataMapped.resolve({
                    results: mappedData,
                    numItems: mappedData.length
                });
            });
            return isDataMapped.promise();
        }).then(function(data) {                    // ***
            debugger;                               // ***
            allQueriesMapped.resolve(data);         // ***
        });
    }));

    return allQueriesMapped.promise();
}

It's inside the map, when it should be outside it:

function multipleQueries(queriesToExecute, rowData) {
    var allQueriesMapped = $.Deferred();

    // If a single query has been provided, convert it into an array
    if (Array.isArray(queriesToExecute) === false) {
        queriesToExecute = [].concat(queriesToExecute);
    }

    // Create a function for each region to run the query.
    $.when.apply($, $.map(queriesToExecute, function(query) {

        // Execute the query in the region
        return $.when(executeQuery(query.InstanceInfo, query.Query)).then(function(data) {
            var isDataMapped = $.Deferred();
            var mappedData = [];
            // Perform some data transformation
            $.when.apply($, $.map(data.results, function(value) {
                var properties = value.succinctProperties;
                //code removed for brevity
                return $.when(mapData(properties)).then(function(mappedRow) {
                    if (mappedRow) {
                        mappedData.push(mappedRow);
                    }
                });
            })).then(function() {
                isDataMapped.resolve({
                    results: mappedData,
                    numItems: mappedData.length
                });
            });
            return isDataMapped.promise();
        });
    })).then(function(data) {
        debugger;
        allQueriesMapped.resolve(data);
    });

    return allQueriesMapped.promise();
}

But there's a lot of unnecessary use of $.when and new $.Deferred in there (see *** 1 comments), and you can wrap your parameter in an array much more simply (see *** 2 comment:

function multipleQueries(queriesToExecute, rowData) {
    // If a single query has been provided, convert it into an array
    if (Array.isArray(queriesToExecute) === false) {
        queriesToExecute = [queriesToExecute]; // *** 2
    }

    // Create a function for each region to run the query.
    return $.when.apply($, $.map(queriesToExecute, function(query) { // *** 1

        // Execute the query in the region
        return executeQuery(query.InstanceInfo, query.Query).then(function(data) { // *** 1
            var mappedData = [];
            // Perform some data transformation
            return $.when.apply($, $.map(data.results, function(value) {
                var properties = value.succinctProperties;
                //code removed for brevity
                return mapData(properties).then(function(mappedRow) { // *** 1
                    if (mappedRow) {
                        mappedData.push(mappedRow);
                    }
                });
            })).then(function() {
                return {
                    results: mappedData,
                    numItems: mappedData.length
                };
            });
        });
    }));
}

When you have a promise already, there's never any need to create a new one via new; just use the one returned by then. Also, when you already have a promise, there's never any need to use $.when(thePromise).

You might also benefit from switching to built-in promise semantics instead of jQuery's Deferred early:

function multipleQueries(queriesToExecute, rowData) {
    // If a single query has been provided, convert it into an array
    if (Array.isArray(queriesToExecute) === false) {
        queriesToExecute = [queriesToExecute];
    }

    // Create a function for each region to run the query.
    return Promise.all(queriesToExecute.map(function(query) {
        // Execute the query in the region
        return executeQuery(query.InstanceInfo, query.Query).then(function(data) {
            return Promise.all(data.results.map(function(value) {
                var properties = value.succinctProperties;
                //code removed for brevity
                return mapData(properties);
            }).then(function(mappedData) {
                mappedData = mappedData.filter(Boolean); // Remove the falsy ones from `mapData`
                return {
                    results: mappedData,
                    numItems: mappedData.length
                };
            });
        });
    }));
}

Promise.all is very useful for dealing with arrays of promises. But be sure you're using an up-to-date jQuery, earlier versions of Deferred didn't interoperate properly with real promises. I don't know (and can't immediately find) when that was fixed.

Note that all of the code above is meant to be *illustrative*. It may need tweaking. — T.J. Crowder, May 01 '18 at 11:18
thanks for this - I am trying to use your second example code to refactor the original code better. However if I put a debugger line just before the return { results: mappedData, numItems: mappedData.length }; this gets hit 3 times when I pass in 3 queries - two times mappedData is empty no results in the query and once it contains data. However in the calling $.when(multipleQueries(stateQueries, rowData)) .then(function (data) { putting a debugger here data is undefined so its as if it is no data returned — Ctrl_Alt_Defeat, May 01 '18 at 11:54
@Ctrl_Alt_Defeat: I'm afraid you'll just have to work through debugging it. Or if you can mock up the various functions and put a runnable [mcve] in the question demonstrating the problem, others can help. To make a runnable example, use the `[<>]` toolbar button; [here's how to do one](https://meta.stackoverflow.com/questions/358992/ive-been-told-to-do-a-runnable-example-with-stack-snippets-how-do-i-do-tha). — T.J. Crowder, May 01 '18 at 12:03
thanks - ill keep looking - moving the then from inside the .map to outside is giving the same result not returning data correctly after all queries have ran - ill keep debugging — Ctrl_Alt_Defeat, May 01 '18 at 12:35

jquery defered returning too early

1 Answers1