3
\$\begingroup\$

I want to download data for all of my repos from the Github API.

Github paginates their responses and includes a header Link type:"next" when there's another page available. So I know I want to wait for all the pages to be received... but I don't know initially how many pages there are going to be, and I don't know if there's a next page until the one before it comes in.

So far I've got this, and it works:

var allRepos = [];
var repoPageReqs = [$.Deferred()]; // <-- sketchy
function receivedReposPage(data, status, req) {

    //handle error inadequately but whatever for now
    if (status !== "success") { console.log(data, status, req); return; }

    console.log("Received repos from Github:", data);
    for (i in data) {
        allRepos.push(data[i]);
    }
    saveRepos(allRepos);

    var headerLink = req.getResponseHeader("Link");
    var headerLinkNext = parseLinkHeader(headerLink)["next"];
    if (headerLinkNext) {
        console.log("Following 'next' header link, collecting paginated repos.");
        repoPageReqs.push(downloadRepos(headerLinkNext));
    } else {
        repoPageReqs[0].resolve(); // <-- resolve sketchiness
    }
}

function downloadRepos(path) {
    return $.get(path, receivedReposPage);
}

function downloadAllRepos(path) {
    repoPageReqs.push(downloadRepos(path));
    return $.when.apply(null, repoPageReqs);
}

$(function () {
    downloadAllRepos("https://api.github.com/my/repos/path")
      .then(function () {
           console.log("finished downloading all repos");
      })
});

The reason I use the sketchy [$.Deferred()] was because the $.when would resolve before the callback pushed the new $.get promise into the promisey array.

Is there a better way to make/resolve this kind of "unraveling" chain?

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Using $.when is useful when you know you have N asynchronous operations that have to resolve. However, in this case you, as you say, don't know how many there'll be, so the simpler thing would be to create one "master" promise/Deferred and resolve that at the appropriate time.

Which is basically what you're already doing with that repoPageReqs[0] promise, you just have it mixed up in an array and insisting on all this $.when stuff that's not useful here.

But first, some small stuff:

  • You have i as an implied global variable in your for(i in data) - that's a no-no.

  • I'm also wary of the parseLinkHeader(headerLink)["next"] line. For one, it might as well be .next instead of the bracket notation, but more importantly, is it safe to assume that parseLinkHeader will always return an object? If it fails, I'd rather have it return null or undefined, but then the code will fail when trying something like null["next"].

    I'd don't know how your parseLinkHeader function works, but it just smells a little strange to me.

Anyway, point is to use a single "overall" promise, instead of all that $.when stuff. Something like:

function downloadAllRepos(path) {
  var deferred = $.Deferred(), // the "master" promise
      repos = [];

  function parseLinkHeader(header) { ... }

  function receivedReposPage(data, status, req) {
    if (status !== 'success') {
      deferred.reject(data, status, req); // reject the promise if any page fails to load
      return;
    }

    // if data is an array, then you should *not* use a for..in loop
    // but a regular for loop instead.  for..in is made to iterate
    // object properties, not array elements. Calling the counter var
    // i makes me think you're dealing with an array... but I don't
    // know what the data variable is, so I've kept the for..in loop
    // and the assumption that it's an object rather than an array,
    // but added a sanity check to skip over any inherited properties
    // that might otherwise get erroneously iterated.
    // (And if data is an array, you might get away with simply
    // saying `repos = repos.concat(data)` instead of looping!)
    for (var prop in data) {
      if (!data.hasOwnProperty(prop)) continue;
      allRepos.push(data[prop]);
    }

    // you may want to call deferred.notify() here to let
    // others know that you've downloaded another page, e.g.
    // you could send the number of repos gotten so far:
    deferred.notify(repos.length);

    // a slightly safer approach to getting/parsing the header
    var linkHeader = parseLinkHeader(req.getResponseHeader('Link'));
    if (linkHeader && linkHeader.next) {
      getPage(linkHeader.next); // get the next page
    } else {
      deferred.resolve(repos); // all done, resolve the promise
    }
  }

  function getPage(path) {
    $.get(path, receivedReposPage);
  }

  // get the first page
  getPage(path);

  // return a promise
  return deferred.promise();
}

// Example
downloadAllRepos("https://api.github.com/my/repos/path")
  .progress(function (repoCount) {
    console.log("Downloaded " + repoCount + " repo(s) so far...");
  })
  .success(function (repos) {
    // code to run when *all* pages have loaded
  })
  .failure(function (data, status, req) {
    // code to run if a page failed to load
    console.log(data, status, req);
  });

I've placed everything inside the downloadAllRepos() function to reduce the overall footprint.

\$\endgroup\$
5
  • \$\begingroup\$ Wow, above and beyond! Simple, tiny, communicative. Bravo! Also, thank you for the early-on "small stuff" pointers, too, those are, as well, very greatly appreciated. :-) \$\endgroup\$
    – irbanana
    Feb 18, 2017 at 13:25
  • \$\begingroup\$ @irbanana Glad you found it useful. FYI, I've just now added a little note in the code regarding the way you're looping through the data you get back. Small stuff, just wanted to make a note of it \$\endgroup\$
    – Flambino
    Feb 18, 2017 at 13:29
  • \$\begingroup\$ And yep, it is an array so I should definitely use for (var x = 0; x < data.length; x++) { data[x] ... }. \$\endgroup\$
    – irbanana
    Feb 18, 2017 at 13:30
  • 1
    \$\begingroup\$ @irbanana Alright, cool. As mentioned, maybe you can skip the loop and just do repos = repos.concat(data) or repos.push.apply(repos, data). Either one should append the new repos to the list if data is an array \$\endgroup\$
    – Flambino
    Feb 18, 2017 at 13:31
  • \$\begingroup\$ Yea! Even better. I had that in there but it was breaking as a side effect (I think) of timing-things not lining up (said repos was null). Will do. \$\endgroup\$
    – irbanana
    Feb 18, 2017 at 13:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.