0

I have some code that will dynamically generate an AJAX request based off a scenario that I'm retrieving via an AJAX request to a server.

The idea is that:

  1. A server provides a "Scenario" for me to generate an AJAX Request.
  2. I generate an AJAX Request based off the Scenario.
  3. I then repeat this process, over and over in a Loop.

I'm doing this with promises here: http://jsfiddle.net/3Lddzp9j/11/

However, I'm trying to edit the code above so I can handle an array of scenarios from the initial AJAX request.

IE:

{
"base": {
    "frequency": "5000"
},
"endpoints": [
    {
        "method": "GET",
        "type": "JSON",
        "endPoint": "https://api.github.com/users/alvarengarichard",
        "queryParams": {
            "objectives": "objective1, objective2, objective3"
        }
    },
    {
        "method": "GET",
        "type": "JSON",
        "endPoint": "https://api.github.com/users/dkang",
        "queryParams": {
            "objectives": "objective1, objective2, objective3"
        }
    }
]

This seems like it would be straight forward, but the issue seems to be in the "waitForTimeout" function.

I'm unable to figure out how to run multiple promise chains. I have an array of promises in the "deferred" variable, but the chain only continues on the first one--despite being in a for loop.

Could anyone provide insight as to why this is? You can see where this is occuring here: http://jsfiddle.net/3Lddzp9j/10/

richie
  • 457
  • 1
  • 6
  • 19
  • 1
    classic for loop without a closure issue. `i` will increment long before the a asynch code completes. Also, $.ajax itself returns a promise so don't really need to create them with `$.Deferred` – charlietfl Mar 03 '15 at 00:30
  • Not sure I understand that. Why would the increment going before the asynch code completes matter, given that I want multiple AJAX calls going at once. Basically, you would continue the .then() chain for each individual scenario....or each part of the deferred array. – richie Mar 03 '15 at 01:17
  • 1
    http://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example – charlietfl Mar 03 '15 at 01:20

4 Answers4

1

I'll try to answer your question using KrisKowal's q since I'm not very proficient with the promises generated by jQuery.

First of all I'm not sure whether you want to solve the array of promises in series or in parallel, in the solution proposed I resolved all of them in parallel :), to solve them in series I'd use Q's reduce

function getScenario() { ... }

function ajaxRequest(instruction) { ... }

function createPromisifiedInstruction(instruction) {
  // delay with frequency, not sure why you want to do this :(
  return Q.delay(instruction.frequency)
    .then(function () {
      return this.ajaxRequest(instruction);
    });
}

function run() {
  getScenario()
    .then(function (data) {
      var promises = [];
      var instruction;
      var i;
      for (i = 0; i < data.endpoints.length; i += 1) {
        instruction = {
          method: data.endpoints[i].method,
          type: data.endpoints[i].type,
          endpoint: data.endpoints[i].endPoint,
          frequency: data.base.frequency
        };
        promises.push(createPromisifiedInstruction(instruction));   
      }
      // alternative Q.allSettled if all the promises don't need to
      // be fulfilled (some of them might be rejected)
      return Q.all(promises);
    })
    .then(function (instructionsResults) {
      // instructions results is an array with the result of each
      // promisified instruction
    })
    .then(run)
    .done();
}

run();

Ok let me explain the solution above:

  1. first of all assume that getScenario gets you the initial json you start with (actually returns a promise which is resolved with the json)
  2. create the structure of each instruction
  3. promisify each instruction, so that each one is actually a promise whose resolution value will be the promise returned by ajaxRequest
  4. ajaxRequest returns a promise whose resolution value is the result of the request, which also means that createPromisifiedInstruction resolution value will be the resolution value of ajaxRequest
  5. Return a single promise with Q.all, what it actually does is fulfill itself when all the promises it was built with are resolved :), if one of them fails and you actually need to resolve the promise anyways use Q.allSettled
  6. Do whatever you want with the resolution value of all the previous promises, note that instructionResults is an array holding the resolution value of each promise in the order they were declared

Reference: KrisKowal's Q

Mauricio Poppe
  • 4,817
  • 1
  • 22
  • 30
1

You have a return statement in the loop in your waitForTimeout function. This means that the function is going to return after the first iteration of the loop, and that is where you are going wrong.

You're also using the deferred antipattern and are using promises in places where you don't need them. You don't need to return a promise from a then handler unless there's something to await.

The key is that you need to map each of your instructions to a promise. Array#map is perfect for this. And please use a proper promise library, not jQuery promises (edit but if you absolutely must use jQuery promises...):

var App = (function ($) {
    // Gets the scenario from the API
    // NOTE: this returns a promise
    var getScenario = function () {
        console.log('Getting scenario ...');
        return $.get('http://demo3858327.mockable.io/scenario');
    };

    // mapToInstructions is basically unnecessary. each instruction does
    // not need its own timeout if they're all the same value, and you're not
    // reshaping the original values in any significant way

    // This wraps the setTimeout into a promise, again
    // so we can chain it
    var waitForTimeout = function(data) {
        var d = $.Deferred();
        setTimeout(function () {
            d.resolve(data.endpoints);
        }, data.base.frequency);
        return d.promise();
    };

    var callApi = function(instruction) {
        return $.ajax({
            type: instruction.method,
            dataType: instruction.type,
            url: instruction.endPoint
        });
    };

    // Final step: call the API from the 
    // provided instructions
    var callApis = function(instructions) {
        console.log(instructions);
        console.log('Calling API with given instructions ...');
        return $.when.apply($, instructions.map(callApi));
    };

    var handleResults = function() {
        var data = Array.prototype.slice(arguments);
        console.log("Handling data ...");
    };

    // The 'run' method
    var run = function() {
        getScenario()
        .then(waitForTimeout)
        .then(callApis)
        .then(handleResults)
        .then(run);
    };

    return {
        run : run
    }
})($);

App.run();
JLRishe
  • 99,490
  • 19
  • 131
  • 169
  • jQuery promises have their issues but are 100% OK for this job. – Roamer-1888 Mar 03 '15 at 09:46
  • @Roamer-1888 As far as I am concerned, they are not 100% OK for _any_ job that involves more than a single promise. The lack of a `Promise.all()` method in itself makes them unideal for this task, setting aside all of the other huge problems they have. – JLRishe Mar 03 '15 at 09:52
  • Not so. In jQuery it's `$.when()`. – Roamer-1888 Mar 03 '15 at 10:01
  • @Roamer-1888 I know about `$.when`, but in order to use it here, you would have to do an awkward `$.when.apply($, instructions.map(callApi));` and then use the `arguments` object in `handleResults`. `Promise.all()` is much cleaner here. – JLRishe Mar 03 '15 at 10:05
  • For sure, a Q solution is more economical of source code but an "awkward" jQuery solution would be way more efficient. `Q.when($.ajax(...))` is expensive. – Roamer-1888 Mar 03 '15 at 11:56
  • Though not detailed in the original question, I don't want to have the extra dependency of Q. Thanks for introducing me to it though, and after looking through it and bluebird (an alternative library) -- they will be very useful for future projects. Thanks! – richie Mar 03 '15 at 21:05
  • @richie Ok, well as Roamer has pointed out the above solution can be relatively easily adapted to jQuery promises (which I have now done) if you absolutely must use them. This answer is almost half as long and (IMHO) quite a bit cleaner than the answer you accepted and referred to as "really elegant". – JLRishe Mar 03 '15 at 21:12
1

Try utilizing deferred.notify within setTimeout and Number(settings.frequency) * (1 + key) as setTimeout duration; msg at deferred.notify logged to console at deferred.progress callback , third function argument within .then following timeout

    var App = (function ($) {
    
        var getScenario = function () {
            console.log("Getting scenario ...");
            return $.get("http://demo3858327.mockable.io/scenario2");
        };
        
        var mapToInstruction = function (data) {
            var res = $.map(data.endpoints, function(settings, key) {
                return {
                    method:settings.method,
                    type:settings.type,
                    endpoint:settings.endPoint,
                    frequency:data.base.frequency
                }
            });
            
            console.log("Instructions recieved:", res);
    
            return res
        };
        
        var waitForTimeout = function(instruction) {        
               var res = $.when.apply(instruction, 
                  $.map(instruction, function(settings, key) {
                    return new $.Deferred(function(dfd) {                    
                        setTimeout(function() {  
                          dfd.notify("Waiting for " 
                                    + settings.frequency 
                                    + " ms")
                         .resolve(settings);                 
                        }, Number(settings.frequency) * (1 + key));
                    }).promise()
                 })
               )
               .then(function() {
                 return this
               }, function(err) {
                 console.log("error", err)
               }
               , function(msg) {
                 console.log("\r\n" + msg + "\r\nat " + $.now() + "\r\n")
               });
               return res
        };
    
        var callApi = function(instruction) {
            console.log("Calling API with given instructions ..."
                       , instruction);
            var res = $.when.apply(instruction, 
              $.map(instruction, function(request, key) {
                return request.then(function(settings) {
                  return $.ajax({
                    type: settings.method,
                    dataType: settings.type,
                    url: settings.endpoint
                  }); 
                })                                                       
              })
            )
            .then(function(data) {
                return $.map(arguments, function(response, key) {              
                    return response[0]
                })
            })
            return res
        };
        
        var handleResults = function(data) {
            console.log("Handling data ..."
                        , JSON.stringify(data, null, 4));
            return data
        };
        
        var run = function() {
            getScenario()
            .then(mapToInstruction)
            .then(waitForTimeout)
            .then(callApi)
            .then(handleResults)
            .then(run);
        };
        
        return {
            // This will expose only the run method
            // but will keep all other functions private
            run : run
        }
    })($);
    
    // ... And start the app
    App.run();
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js">
</script>

jsfiddle http://jsfiddle.net/3Lddzp9j/13/

guest271314
  • 1
  • 15
  • 104
  • 177
1

The main problems are that :

  • waitForTimeout isn't passing on all the instructions
  • even if waitForTimeout was fixed, then callApi isn't written to perform multiple ajax calls.

There's a number of other issues with the code.

  • you really need some data checking (and associated error handling) to ensure that expected components exist in the data.
  • mapToInstruction is an unnecessary step - you can map straight from data to ajax options - no need for an intermediate data transform.
  • waitForTimeout can be greatly simplified to a single promise, resolved by a single timeout.
  • synchronous functions in a promise chain don't need to return a promise - they can return a result or undefined.

Sticking with jQuery all through, you should end up with something like this :

var App = (function ($) {
    // Gets the scenario from the API
    // sugar for $.ajax with GET as method - NOTE: this returns a promise
    var getScenario = function () {
        console.log('Getting scenario ...');
        return $.get('http://demo3858327.mockable.io/scenario2');
    };

    var checkData = function (data) {
        if(!data.endpoints || !data.endpoints.length) {
            return $.Deferred().reject('no endpoints').promise();
        }
        data.base = data.base || {};
        data.base.frequency = data.base.frequency || 1000;//default value
    };

    var waitForTimeout = function(data) {
        return $.Deferred(function(dfrd) {
            setTimeout(function() {
                dfrd.resolve(data.endpoints);
            }, data.base.frequency);
        }).promise();
    };

    var callApi = function(endpoints) {
        console.log('Calling API with given instructions ...');
        return $.when.apply(null, endpoints.map(ep) {
            return $.ajax({
                type: ep.method,
                dataType: ep.type,
                url: ep.endpoint
            }).then(null, function(jqXHR, textStatus, errorThrown) {
                return textStatus;
            });
        }).then(function() {
            //convert arguments to an array of results
            return $.map(arguments, function(arg) {
                return arg[0];
            });
        });
    };

    var handleResults = function(results) {
        // results is an array of data values/objects returned by the ajax calls.
        console.log("Handling data ...");
        ...
    };

    // The 'run' method
    var run = function() {
        getScenario()
        .then(checkData)
        .then(waitForTimeout)
        .then(callApi)
        .then(handleResults)
        .then(null, function(reason) {
            console.error(reason);
        })
        .then(run);
    };

    return {
        run : run
    }
})(jQuery);

App.run();

This will stop on error but could be easily adapted to continue.

Roamer-1888
  • 19,138
  • 5
  • 33
  • 44
  • Thanks for all the context around why my current iteration wasn't working. I have this up here: http://jsfiddle.net/858m3mL5/1/ ... I fixed some syntax errors, but currently seeing some strange behavior here. Basically, after i .map() the endpoints ...I'm unable to apply the correct object properties to the AJAX request. Not sure why this is the case, any insight would be helpful. – richie Mar 03 '15 at 22:47
  • Ahh, realized it was just a typo in the URL property of the AJAX request. You can see it fully working here: http://jsfiddle.net/858m3mL5/1/ ...thanks again for the solution. Uses jQuery (no need for extra dependencies) and sticks pretty close to my original structure. – richie Mar 03 '15 at 23:00
  • Ah right, typo. Good error reporting should help with that sort of error and others. – Roamer-1888 Mar 04 '15 at 16:29
  • Speaking of error reporting...in this example here: http://jsfiddle.net/858m3mL5/2/. I'm trying to pass along an error from the original $.get, why doesn't .reject() work there like it does in the checkData function below. Also the .then() statement within the run function (where you log the error reason), is there any particular reason you use .then() and not .fail() ? – richie Mar 04 '15 at 19:43
  • jQuery's `promise.done()` and `promise.error()` handlers are transparent to their input promise - it is guaranteed to pass straight through with the same state and the same data regardless of what the callbacks do/return. Only `promise.then()` has "filtering" power - it is guaranteed to return a new promise, the state/data of which is determined by what then's handler(s) return. – Roamer-1888 Mar 04 '15 at 20:28
  • Got it, out of curiosity what would be the correct approach to continuing on an error in the callAPI function. We use .map to create multiple AJAX request, but what about if a single one fails--how would I go about allowing the loop to continue despite a single AJAX call failing (for one reason or another) – richie Mar 05 '15 at 01:30
  • I know I can use .always(), where we map the arguments to an array of results, so that runs despite a failure in one of the requests. The issue however, is that the $.map function throws an error due to the undefined nature of the arguments. – richie Mar 05 '15 at 01:55