0

Before I get started, I want to mention that I don't have much experience with promises/deferred objects in JavaScript.

What I'm trying to achieve, is when a user clicks a button, it runs multiple $.getJSON requests, handles the responses and updates the page accordingly.

I also need to notify the user when all of these requests have been completed, here is the connect method I have which handles the first part I spoke about:

function connect(row) {
    return new Promise(function(resolve, reject) {
        var trees = $("#selectedTree").val();
        var employee_id = row.attr("rel");

        var columns = row.children();
        var emailColumn = $(columns[1]);
        var firstNameColumn = $(columns[2]);
        var lastNameColumn = $(columns[3]);
        var jobTitleColumn = $(columns[4]);
        var statusColumn = $(columns[5]);
        var actionsColumn = $(columns[6]);

        actionsColumn.html("<span class='spinner'><i class='fa fa-spinner fa-spin'></i></span>");
        $.getJSON("functions/connect_with.php", {
            employee_id: employee_id,
            trees: trees
        }, function(response) {
            emailColumn.html(response.message.person.email);
            actionsColumn.html("<i class='text-success fa fa-check'></i>");
            if(response.success) {
                firstNameColumn.html(response.message.person.name.givenName);
                lastNameColumn.html(response.message.person.name.familyName);
                jobTitleColumn.html(response.message.person.employment.title);
            }
            statusColumn.html("<i class='text-success fa fa-sitemap'></i>");
            resolve(true);
        });
    });
}

And this is what happens when the user clicks said button:

    $("#connectAll").click(function() {
        alert("OFF WE GO");
        $(this).hide();
        var methods = [];
        $(".staffMember input:checked").each(function(index, checkbox) {
            checkbox = $(checkbox);
            var row = checkbox.parents("tr");
            methods.push(connect(row));
        });
        $.when.apply($, methods).then(function () {
            alert("WE DID IT MOM");
            $("#connectAll").show();   
        });
    });

However, both alerts are sent immediately after one another, and is not waiting for the requests to complete. I've tried other methods of doing this and I can't seem to get it right.

ThePerplexedOne
  • 2,920
  • 15
  • 30

2 Answers2

2

Rather than calling apply use Promise.all

This executes an array of promises and returns once all have completed.

So your code would become:

$("#connectAll").click(function() {
  alert("OFF WE GO");
  $(this).hide();
  var methods = [];
  $(".staffMember input:checked").each(function(index, checkbox) {
    checkbox = $(checkbox);
    var row = checkbox.parents("tr");
    methods.push(connect(row));
  });
  Promise.all(methods).then(values => {
    alert("WE DID IT MOM");
    $("#connectAll").show();   
  }).catch(reason => {
    alert("MOM, SOMETHING WENT WRONG");
  });
});
timothyclifford
  • 6,799
  • 7
  • 57
  • 85
  • In jQuery context it's much better to recommend `$.when` over `Promise.all`. It works the same (except that you need `.apply()` because it expects a parameter list, not an array) – but it also works in browsers that do not have native promise support. – Tomalak Nov 14 '16 at 16:00
  • Then what would I return in the `connect` method? A deferred object or...? @Tomalak – ThePerplexedOne Nov 14 '16 at 16:01
  • You would return the Ajax call result (`return $.getJSON(...)`) - those are already promises in jQuery, you don't need that `new Promise()` call at all. – Tomalak Nov 14 '16 at 16:03
  • @Tomalak I would agree, if these were jQuery promises, but they are not. but yea, no reason to use new Promise. – Kevin B Nov 14 '16 at 16:21
  • @KevinB They are real promises for jQuery 3.0 and above (http://stackoverflow.com/questions/23951745/is-any-jquery-version-compliant-to-promise-a-specifications). And in older versions, when technically they aren't 100% compliant, they still are good enough for this kind of work. – Tomalak Nov 14 '16 at 16:28
1

You don't need to use Promise in your code at all - and, until all browsers support native promises, I would advise against it unless you are loading your own promise library.

But the main reason is that jQuery implements promises to a good-enough degree for this task. Every jQuery Ajax function returns a promise, you can use them directly.

Here is how I would write your code:

function connect($row) {
    var columns = $row.children("td");
    $(columns[6]).html("<span class='spinner'><i class='fa fa-spinner fa-spin'></i></span>");

    return $.getJSON("functions/connect_with.php", {
        employee_id: $row.attr("rel"),
        trees: $("#selectedTree").val()
    }).done(function (response) {
        $(columns[1]).text(response.message.person.email);
        if(response.success) {
            $(columns[2]).text(response.message.person.name.givenName);
            $(columns[3]).text(response.message.person.name.familyName);
            $(columns[4]).text(response.message.person.employment.title);
        }
        $(columns[5]).html("<i class='text-success fa fa-sitemap'></i>");
    }).always(function () {
        $(columns[6]).html("<i class='text-success fa fa-check'></i>");
    });
}


$("#connectAll").click(function() {
    $(this).hide();

    var calls = $(".staffMember input:checked").map(function () {
        return connect( $(this).parents("tr") );
    }).toArray();

    $.when.apply($, calls)
    .fail(function (jqXhr, status, error) {
        // display or handle the error
    })
    .always(function () {
        $("#connectAll").show();
    });
});

Side notes:

  • You should implement an error handler (.fail(...))
  • Code that removes spinners or restores buttons should sit in the .always() handler, so that page functionality isn't lost because of an intermittent Ajax error.
  • You should use jQuery's .text() (as opposed to .html()) to set text values.
Tomalak
  • 332,285
  • 67
  • 532
  • 628