-1

Hello I am continuing to learn promises and am struggling with how to correctly use Promise.All or whether or not I should be using it in this situation.

I am connecting to a data source and resolving my first promise to an array. After that I want to take those results and with another promise loop through the results and add another value to the array which is using an external API call.

After research it sounded like I needed to use Promise.All so that the promises would be completed before returning my response but I am still getting a status of pending on the value that I am adding to my array instead of the expected value. Again I am just learning node and promises so my approach might be completely flawed and in that case I am open to any suggestions on a better way to do what I am attempting to accomplish.

function GetSearches(req, res) { 

    var BingSearches = new Promise((resolve, reject) => {
        GetBingSearches().toArray((err, response)=>{
            resolve(response);
        });          
    }) /* get data from external data source example data: [{SearchTerm: "Boxing", id: '4c142f92-ba6d-46af-8dba-cb88ebabb94a', CreateDate: '2017-08-10T13:59:20.534-04:00'}] */


    var GetSearchesWithImageUrl = BingSearches.then((response,reject) =>{
        for(var i in response){
            response[i].Image = new Promise((resolve, reject) => { 
                Bing.images(response[i].SearchTerm, {
                count: 1
                }, (err,res,body) => {
                    const bingImageUrl = body.value[0].contentUrl;
                    console.log(bingImageUrl) // bing image url link
                    resolve(bingImageUrl);
                });
            });
        } // loop through array and add value

        return response;
    }).catch((err) => {
        console.log(err);
    })

    return Promise.all([BingSearches ,GetSearchesWithImageUrl ]).then(([BingSearches , GetSearchesWithImageUrl ]) => {
        console.log(GetSearchesWithImageUrl )
         /* my response is now [{SearchTerm: "Boxing", id: '4c142f92-ba6d-46af-8dba-cb88ebabb94a', CreateDate: '2017-08-10T13:59:20.534-04:00', Image: Promise { pending } }] */

        res.status(200).json(GetSearchesWithImageUrl )
    }).catch((err) => {
        console.log(err);
    })
}

I was hoping that Promise.All would wait for all my promises to complete but I do not seem to understand how it works. I am open to any advice/explanation of what I am doing wrong.

Thanks!

dev53
  • 404
  • 10
  • 26
  • 2
    You're calling `return Promise.all([Searches,Images])`, but I don't see either `Searches` or `Images` defined anywhere. What are they? – jfriend00 Aug 29 '17 at 02:56
  • in `Promise.all([Searches,Images])` it seems that Searches and Images are undefined, no? – Jaromanda X Aug 29 '17 at 02:56
  • 2
    Should that be `Promise.all([BingSearches, GetSearchesWithImageUrl])` perhaps – Jaromanda X Aug 29 '17 at 02:57
  • @JaromandaX - `GetSearchesWithImageUrl` does not wait for images to resolve so that would not be enough. This code is pretty far off. – jfriend00 Aug 29 '17 at 02:58
  • With assuming your typo for Promise.all([BingSearches, GetSearchesWithImageUrl]), I would say that you can't expect promise chain to satisfy if a function call doesn't return promise. You have to return the async calls from respective fns – binariedMe Aug 29 '17 at 02:58
  • 1
    This code looks pretty far off. Please describe "in words" what you are trying to achieve (without us having to guess) so we can suggest a fixed up version. – jfriend00 Aug 29 '17 at 02:59
  • true @jfriend00 - I meant is that OP is **actually** using - because as shown, it makes even less sense – Jaromanda X Aug 29 '17 at 02:59
  • [Don't use `for…in` enumerations on arrays!](https://stackoverflow.com/q/500504/1048572) – Bergi Aug 29 '17 at 03:06
  • 1
    Also, since operations should be promisified at the lowest level, we probably need to know what `GetBingSearches()` is doing to offer the best answer. – jfriend00 Aug 29 '17 at 03:15
  • Thanks for the comments everyone! Sorry I was pulled away from this for a minute. @binariedMe is correct. When I was trying to clean things up to make it easier to understand what each promise was doing I changed the name and made a typo and forgot to also change it in the Promise.All! – dev53 Aug 29 '17 at 12:26
  • @jfriend00 In words I want to get data from my first data source (a list of searches) then I want to take the search term from each row of data and use that as a parameter in my Bing.images(searchTerm) API call (basically getting the image for the search). Hopefully that makes sense. Sorry for not being clear in my question! – dev53 Aug 29 '17 at 12:29

1 Answers1

1

Here's my best stab at this problem:

// Promisified search, resolves to response array
function getBingSearchesPromise() {
    return new Promise((resolve, reject) => {
        GetBingSearches().toArray((err, response) => {
            if (err) {
                reject(err);
            } else {
                resolve(response);
            }
        });          
    });
}

// get first image url for a search term, resolves to an image URL
function getBingImagePromise(term) {
    return new Promise((resolve, reject) => {
        Bing.images(term, {count: 1}, (err, res, body) => {
            if (err) {
                reject(err);
            } else {
                resolve(body.value[0].contentUrl);
            }
        });
    });
}


// no need to pass req or res into here
// returns promise, resolves to array of images URLs
function GetSearches() {
    return getBingSearchesPromise().then(searchResponses => {
        // get all images
        let promises = searchResponses.map(searchItem => {
            return getBingImagePromise(searchItem.SearchTerm);
        });
        return Promise.all(promises);
    })    
}

// then inside your request handler where you have req and res, you can do this
GetSearches().then(images => {
    res.json(images);
}).catch(err => {
    console.log(err);
    res.status(500).end();
});

Because your code was pretty far off and did not include a words description of what you are trying to do, this is an educated guess about you're trying to accomplish. If I got the objective wrong with my guess, you can either just learn from this and apply it to your actual problem or I can delete my answer.

Summary of Changes:

  1. Promisify async operations separately, then build all logic with promises (no plain callbacks). This could probably be done even better if we could see the code for GetBingSearches() which should probably have some arguments passed into it rather than be hard-wired to do a specific type of search.

  2. Get the searches promise and wait for it with .then() to get an array of results.

  3. In that .then() handler, use .map() to fetch each image URL and create an array of promises.

  4. Wait for that array of promises with Promise.all().

  5. Separate out req and res from this code because only the final response needs to use res so this way you code can be more usable and you can just call this code from the response handler and get the result without passing res into it.

  6. Add error handling for all errors. Propagate all errors back up the chain.

  7. Never use for/in to iterate an array (it iterates properties of an object, not just array elements). If your desired result is a 1-to-1 array, then .map() is perfect for that. Otherwise, in ES6, use for/of to iterate an array.

jfriend00
  • 683,504
  • 96
  • 985
  • 979
  • This is awesome. I appreciate the summary and the attempt to get me pointed in the right direction. This is a lot cleaner and really helps me understand the flow of the promises and how they chain together. I think the only thing I need to figure out now is how the .map works with just adding the image to my array. Something like `searchResponses.map(searchItem => { searchItem.bingImageUrl = getBingImagePromise(searchItem.SearchTerm); return searchItem });` – dev53 Aug 29 '17 at 20:24
  • I figured it out... I used .then on my Promise.All to add the response to my array. Again thank you for the explanation. This really helped. – dev53 Aug 29 '17 at 21:01