7

I have the following functions with promises:

const ajaxRequest = (url) => {
  return new Promise(function(resolve, reject) {
    axios.get(url)
      .then((response) => {
        //console.log(response);
        resolve(response);
      })
      .catch((error) => {
        //console.log(error);
        reject();
      });
  });
}


const xmlParser = (xml) => {
  let { data } = xml;
  return new Promise(function(resolve, reject) {
    let parser = new DOMParser();
    let xmlDoc = parser.parseFromString(data,"text/xml");

    if (xmlDoc.getElementsByTagName("AdTitle").length > 0) {
      let string = xmlDoc.getElementsByTagName("AdTitle")[0].childNodes[0].nodeValue;
      resolve(string);
    } else {
      reject();
    }
  });
}

I'm trying to apply those functions for each object in array of JSON:

const array = [{"id": 1, "url": "www.link1.com"}, {"id": 1, "url": "www.link2.com"}]

I came up with the following solution:

function example() {
    _.forEach(array, function(value) {
        ajaxRequest(value.url)
            .then(response => {
                xmlParser(response)
            .catch(err => {
                console.log(err);
            });
        });
    }
 }

I was wondering if this solution is acceptable regarding 2 things:

  1. Is it a good practice to apply forEach() on promises in the following matter.

  2. Are there any better ways to pass previous promise results as parameter in then() chain? (I'm passing response param).

wizard
  • 583
  • 2
  • 9
  • 26
  • I think it's fine to use promises in a loop like you are doing ; it won't bite. not sure if there is a better way in fact – Timothy Groote Jul 25 '17 at 07:29
  • I personally think that there are no particular problem with your code. It looks OK. What I might do is: 1. Use `Promise.all()` in order to execute all AJAX calls together, and then `_.map` the results to the `xmlParser`. But your code looks good, IMHO. BTW, +1 for the nice question. – Catalyst Jul 25 '17 at 07:29
  • You don’t need lodash, simply use for (var i in array) { ... } – Arthur Guiot Jul 25 '17 at 08:00
  • Avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! – Bergi Jul 25 '17 at 09:42
  • Your parenthesis look very mismatched. Can you fix them (and/or the indentation) to show us what you are actually doing? – Bergi Jul 25 '17 at 09:44
  • It's almost *never* good practise to use `forEach`. Especially with promises where you want to get a result to await back. – Bergi Jul 25 '17 at 09:46

4 Answers4

5

You can use .reduce() to access previous Promise.

function example() {
    return array.reduce((promise, value) =>
       // `prev` is either initial `Promise` value or previous `Promise` value
       promise.then(prev => 
         ajaxRequest(value.url).then(response => xmlParser(response))
       )
    , Promise.resolve())
 }
 // though note, no value is passed to `reject()` at `Promise` constructor calls
 example().catch(err => console.log(err)); 

Note, Promise constructor is not necessary at ajaxRequest function.

const ajaxRequest = (url) => 
    axios.get(url)
      .then((response) => {
        //console.log(response);
        return response;
      })
      .catch((error) => {
        //console.log(error);
      });
guest271314
  • 1
  • 15
  • 104
  • 177
  • why Promise constructor is not necessary? – wizard Jul 25 '17 at 07:47
  • @wizard `axios.get` already returns a Promise, so you don't need to explicitly create a new one. – CodingIntrigue Jul 25 '17 at 07:48
  • @guest271314 correct me if I wrong, but the `response` is the returned value after the promise is fulfilled and not the promise itself. Indeed, I get `Cannot read property 'then' of undefined` error in example() function. – wizard Jul 25 '17 at 08:04
1

The only issue with the code you provided is that result from xmlParser is lost, forEach loop just iterates but does not store results. To keep results you will need to use Array.map which will get Promise as a result, and then Promise.all to wait and get all results into array.

I suggest to use async/await from ES2017 which simplifies dealing with promises. Since provided code already using arrow functions, which would require transpiling for older browsers compatibility, you can add transpiling plugin to support ES2017.

In this case your code would be like:

function example() {
  return Promise.all([
    array.map(async (value) => {
      try {
        const response = await ajaxRequest(value.url);
        return xmlParser(response);
      } catch(err) {
        console.error(err);
      }
    })
  ])
}

Above code will run all requests in parallel and return results when all requests finish. You may also want to fire and process requests one by one, this will also provide access to previous promise result if that was your question:

async function example(processResult) {
  for(value of array) {
    let result;
    try {
      // here result has value from previous parsed ajaxRequest.
      const response = await ajaxRequest(value.url);
      result = await xmlParser(response);
      await processResult(result);
    } catch(err) {
      console.error(err);
    }
  }
}
Max Vorobjev
  • 1,233
  • 6
  • 7
  • How does Answer resolve Questions at OP: _"while access previous promise results in a .then() chain?"_ , _"to pass previous promise results as parameter in then() chain?"_ ? Unless by "previous promise" OP means `ajaxRequest(value.url)` as to `xmlParser(response)` instead of previous `Promise` value of chained `ajaxRequest(value.url)` and `xmlParser(response)` from previous element of `array`. – guest271314 Jul 25 '17 at 08:34
  • That's how I got it, @wizard wants to chain `ajaxRequest` and `xmlParser`, just looking for better ways. Also his initial `array` has urls which are logical inputs for `ajaxRequests`. The issue with `example()` is that he does not store result from xmlParser anywhere. Also using ES6 features, his code is one step away of using ES2017. – Max Vorobjev Jul 25 '17 at 08:52
  • @guest271314, I don't see that OP's passing the previous promise value to the next one. I think that he didn't word his intentions correctly. – Catalyst Jul 25 '17 at 08:55
  • @Catalyst _"I think that he didn't word his intentions correctly."_ That could be possible – guest271314 Jul 25 '17 at 09:01
1

Another solution is using Promise.all for doing this, i think is a better solution than looping arround the ajax requests.

const array = [{"id": 1, "url": "www.link1.com"}, {"id": 1, "url": "www.link2.com"}]

function example() {
    return Promise.all(array.map(x => ajaxRequest(x.url)))
        .then(results => {
            return Promise.all(results.map(data => xmlParser(data)));
        });
}

example().then(parsed => {
    console.log(parsed); // will be an array of xmlParsed elements
});
nicowernli
  • 3,250
  • 22
  • 37
0

Are there any better ways to pass previous promise results as parameter in then() chain?

In fact, you can chain and resolve promises in any order and any place of code. One general rule - any chained promise with then or catch branch is just new promise, which should be chained later.

But there are no limitations. With using loops, most common solution is reduce left-side foldl, but you also can use simple let-variable with reassign with new promise.

For example, you can even design delayed promises chain:

function delayedChain() {
  let resolver = null
  let flow = new Promise(resolve => (resolver = resolve));
  for(let i=0; i<100500; i++) {
    flow = flow.then(() => {
      // some loop action
    })
  }

  return () => {
    resolver();
    return flow;
  }
}

(delayedChain())().then((result) => {
  console.log(result)
})
Vladislav Ihost
  • 2,127
  • 11
  • 26