2

In my code below I get an empty array on my console.log(response) but the console.log(filterdIds) inside the getIds function is showing my desired data. I think my resolve is not right.

Note that I run do..while once for testing. The API is paged. If the records are from yesterday it will keep going, if not then the do..while is stopped.

Can somebody point me to the right direction?

const axios = require("axios");

function getToken() {
    // Get the token
}

function getIds(jwt) {
    return new Promise((resolve) => {
        let pageNumber = 1;
        const filterdIds = [];

        const config = {
            //Config stuff
        };

        do {
            axios(config)
                .then((response) => {
                    response.forEach(element => {
                        //Some logic, if true then:
                        filterdIds.push(element.id);
                        console.log(filterdIds);
                    });
                })
                .catch(error => {
                    console.log(error);
                });
        } while (pageNumber != 1)
        resolve(filterdIds);
    });
}


getToken()
    .then(token => {
        return token;
    })
    .then(jwt => {
        return getIds(jwt);
    })
    .then(response => {
        console.log(response);
    })
    .catch(error => {
        console.log(error);
    });

I'm also not sure where to put the reject inside the getIds function because of the do..while.

ggorlen
  • 44,755
  • 7
  • 76
  • 106
RemcoE33
  • 1,551
  • 1
  • 4
  • 11
  • 1
    Unsure why you have a `do/while` loop that will execute exactly one time. – codemonkey Mar 06 '21 at 19:23
  • Also, what is `filterdShipmentsIds`? I don't see that variable anywhere. Did you mean `resolve(filterdIds);`? – codemonkey Mar 06 '21 at 19:26
  • [Why is my variable unaltered after I modify it inside of a function? - Asynchronous code reference](https://stackoverflow.com/q/23667086) – VLAZ Mar 06 '21 at 19:26
  • Also relevant: [What is the explicit promise construction antipattern and how do I avoid it?](https://stackoverflow.com/q/23803743) – VLAZ Mar 06 '21 at 19:28
  • Hi @codemonkey thank you for your reply. I made a edit to my post. – RemcoE33 Mar 06 '21 at 19:28
  • `resolve(filterdIds);` will resolve with an empty array because the `axios` call is not done by that point. Just move `resolve(filterdIds);` right below the `forEach`, it will work like a charm. – codemonkey Mar 06 '21 at 19:30
  • @codemonkey, thank you! That is working. I'm still a bit unsure why, because of the do..while. This is trowing of my thinking. In my thinking first complete the do..while to get all the id's from let say 3 pages, then resolve. – RemcoE33 Mar 06 '21 at 19:36
  • @codemonkey that only works in OP's contrived example of one page, but it won't help when there are multiple pages in the loop because `resolve` will already have been called by the time the second result arrives. – ggorlen Mar 06 '21 at 20:32
  • @ggorlen I am perfectly aware of it. That's also why I didn't want to post an answer as I thought there were still too many unknowns. – codemonkey Mar 06 '21 at 20:35

2 Answers2

3

The fundamental problem is that resolve(filterdIds); runs synchronously before the requests fire, so it's guaranteed to be empty.

Promise.all or Promise.allSettled can help if you know how many pages you want up front (or if you're using a chunk size to make multiple requests--more on that later). These methods run in parallel. Here's a runnable proof-of-concept example:

const pages = 10; // some page value you're using to run your loop

axios
  .get("https://httpbin.org") // some initial request like getToken
  .then(response => // response has the token, ignored for simplicity
    Promise.all(
      Array(pages).fill().map((_, i) => // make an array of request promisess
        axios.get(`https://jsonplaceholder.typicode.com/comments?postId=${i + 1}`)
      )
    )
  )
  .then(responses => {
    // perform your filter/reduce on the response data
    const results = responses.flatMap(response =>
      response.data
        .filter(e => e.id % 2 === 0) // some silly filter
        .map(({id, name}) => ({id, name}))
    );
    
    // use the results
    console.log(results);
  })
  .catch(err => console.error(err))
;
<script src="https://unpkg.com/axios/dist/axios.min.js"></script>

The network tab shows the requests happening in parallel:

parallel request waterfall

If the number of pages is unknown and you intend to fire requests one at a time until your API informs you of the end of the pages, a sequential loop is slow but can be used. Async/await is cleaner for this strategy:

(async () => {
  // like getToken; should handle err
  const tokenStub = await axios.get("https://httpbin.org");
  
  const results = [];
  
  // page += 10 to make the snippet run faster; you'd probably use page++
  for (let page = 1;; page += 10) {
    try {
      const url = `https://jsonplaceholder.typicode.com/comments?postId=${page}`;
      const response = await axios.get(url);
      
      // check whatever condition your API sends to tell you no more pages
      if (response.data.length === 0) { 
        break;
      }
      
      for (const comment of response.data) {
        if (comment.id % 2 === 0) { // some silly filter
          const {name, id} = comment;
          results.push({name, id});
        }
      }
    }
    catch (err) { // hit the end of the pages or some other error
      break;
    }
  }
  
  // use the results
  console.log(results);
})();
<script src="https://unpkg.com/axios/dist/axios.min.js"></script>

Here's the sequential request waterfall:

sequential request waterfall

A task queue or chunked loop can be used if you want to increase parallelization. A chunked loop would combine the two techniques to request n records at a time and check each result in the chunk for the termination condition. Here's a simple example that strips out the filtering operation, which is sort of incidental to the asynchronous request issue and can be done synchronously after the responses arrive:

(async () => {  
  const results = [];
  const chunk = 5;
  
  for (let page = 1;; page += chunk) {
    try {
      const responses = await Promise.all(
        Array(chunk).fill().map((_, i) => 
          axios.get(`https://jsonplaceholder.typicode.com/comments?postId=${page + i}`)
        )
      );
      
      for (const response of responses) {      
        for (const comment of response.data) {
          const {name, id} = comment;
          results.push({name, id});
        }
      }
      
      // check end condition
      if (responses.some(e => e.data.length === 0)) { 
        break;
      }
    }
    catch (err) {
      break;
    }
  }
  
  // use the results
  console.log(results);
})();
<script src="https://unpkg.com/axios/dist/axios.min.js"></script>

chunked request waterfall

(above image is an except of the 100 requests, but the chunk size of 5 at once is visible)

Note that these snippets are proofs-of-concept and could stand to be less indiscriminate with catching errors, ensure all throws are caught, etc. When breaking it into sub-functions, make sure to .then and await all promises in the caller--don't try to turn it into synchronous code.

See also

ggorlen
  • 44,755
  • 7
  • 76
  • 106
  • Thank you for your anwser. Unfortunately i don’t know the amount of pages. It is a API that has the last order on top. I get 50 results back on each page. Depending on the amount of orders that day i have different amount of pages. That is why i have a do while and when i hit a date lower then yesterday to do..while is broken. – RemcoE33 Mar 06 '21 at 20:07
  • The bottom example basically shows this--just use `for (let page = 1;; pages++) {}` and break the loop when it throws or your response status/data indicates no more results are available. There's nothing special about `do`-`while`--it's just a more confusing way to write loops that isn't used often. See the update. – ggorlen Mar 06 '21 at 20:13
  • Thank you for the async await version. And your insight. Will read all the references in this topic. – RemcoE33 Mar 06 '21 at 20:46
  • No problem and stick with it--promises take some time to grasp. – ggorlen Mar 06 '21 at 20:58
1

To take a step back and think about why you ran into this issue, we have to think about how synchronous and asynchronous javascript code works together. Your synchronous getIds function is going to run to completion, stepping through each line until it gets to the end.

The axios function invocation is returning a Promise, which is an object that represents some future fulfillment or rejection value. That Promise isn't going to resolve until the next cycle of the event loop (at the earliest), and your code is telling it to do some stuff when that pending value is returned (which is the callback in the .then() method).

But your main getIds function isn't going to wait around... it invokes the axios function, gives the Promise that is returned something to do in the future, and keeps going, moving past the do/while loop and onto the resolve method which returns a value from the Promise you created at the beginning of the function... but the axios Promise hasn't resolved by that point and therefore filterIds hasn't been populated.

When you moved the resolve method for the promise you're creating into the callback that the axios resolved Promise will invoke, it started working because now your Promise waits for axios to resolve before resolving itself.

Hopefully that sheds some light on what you can do to get your multi-page goal to work.


I couldn't help thinking there was a cleaner way to allow you to fetch multiple pages at once, and then recursively keep fetching if the last page indicated there were additional pages to fetch. You may still need to add some additional logic to filter out any pages that you batch fetch that don't meet whatever criteria you're looking for, but this should get you most of the way:

async function getIds(startingPage, pages) {
    const pagePromises = Array(pages).fill(null).map((_, index) => {
        const page = startingPage + index;
        // set the page however you do it with axios query params
        config.page = page;
        return axios(config);
    });

    // get the last page you attempted, and if it doesn't meet whatever
    // criteria you have to finish the query, submit another batch query
    const lastPage = await pagePromises[pagePromises.length - 1];
    
    // the result from getIds is an array of ids, so we recursively get the rest of the pages here
    // and have a single level array of ids (or an empty array if there were no more pages to fetch)
    const additionalIds = !lastPage.done ? [] : await getIds(startingPage + pages, pages);

    // now we wait for all page queries to resolve and extract the ids
    const resolvedPages = await Promise.all(pagePromises);
    const resolvedIds = [].concat(...resolvedPages).map(elem => elem.id);

    // and finally merge the ids fetched in this methods invocation, with any fetched recursively
    return [...resolvedIds, ...additionalIds];
}
knappsacks
  • 106
  • 7
  • @ggorlen provided an code snipit solution. But because of your explanation i finally understand why it was not working. I never realized the getIds is not waiting on the axios calls. I read multiple references in this post but this is by far the most simple to follow. Thank you so much! – RemcoE33 Mar 06 '21 at 20:52
  • @remcoE33 I'll take understanding a concept over a specific example any day. :) Happy to help. – knappsacks Mar 07 '21 at 11:24
  • @remcoE33, also if you did find the answer helpful, an up-vote would be appreciated. :) – knappsacks Mar 07 '21 at 23:46
  • @RemcoE33 I added a recursive example that might be simpler to use. – knappsacks Mar 09 '21 at 12:24