0

I have a set of functions in Node.js that I would like to load in a certain order. I will provide some mockup code abstracted and simplified:

function updateMyApp() {
loadDataToServer()
.then(() => useData())
.then(() => saveData())
.then(() => { console.log("updateMyApp done") })
}

function loadDataToServer() {
return new Promise( (resolve, reject) {
...preparing data and save file to cloud...
resolve()})
}

function handleDataItem(item) {
// Function that fetches data item from database and updates each data item
console.log("Name", item.name)
}

function saveData() {
// Saves the altered data to some place
}

useData is a bit more complex. In it I would like to, in order:

  1. console.log('Starting alterData()')
  2. Load data, as json, from the cloud data source
  3. Iterate through every item in the json file and do handleDataItem(item) on it.
  4. When #2 is done -> console.log('alterData() done')
  5. Return a resolved promise back to updateMyApp
  6. Go on with saveData() with all data altered.

I want the logs to show:

Starting useData()
Name: Adam
Name: Ben
Name: Casey
useData() done

my take on this is the following:

function useData() {
   console.log('Starting useData()')
   return new Promise( function(resolve, reject) {
      readFromCloudFileserver()
      .then(jsonListFromCloud) => {
         jsonListFromCloud.forEach((item) => {
            handleDataItem(item)
         }
      })
      .then(() => {
         resolve() // I put resolve here because it is not until everything is finished above that this function is finished
         console.log('useData() done')
      }).catch((error) => { console.error(error.message) })
   })
}

which seems to work but, as far as I understand this is not how one is supposed to do it. Also, this seems to do the handleDataItem outside of this chain so the logs look like this:

Starting useData()
useData() done
Name: Adam
Name: Ben
Name: Casey

In other words. It doesn't seem like the handleDataItem() calls are finished when the chain has moved on to the next step (.then()). In other words, I can not be sure all items have been updated when it goes on to the saveData() function?

If this is not a good way to handle it, then how should these functions be written? How do I chain the functions properly to make sure everything is done in the right order (as well as making the log events appear in order)?

Edit: As per request, this is handleDataItem less abstracted.

function handleDataItem(data) {

   return new Promise( async function (resolve) {  
      data['member'] = true 
      if (data['twitter']) {
         const cleanedUsername = twitterApi.cleanUsername(data['twitter']).toLowerCase()       

         if (!data['twitter_numeric']) {
            var twitterId = await twitterApi.getTwitterIdFromUsername(cleanedUsername)
            if (twitterId) {
               data['twitter_numeric'] = twitterId
            }
         }
         if (data['twitter_numeric']) {         
            if (data['twitter_protected'] != undefined) {
               var twitterInfo = await twitterApi.getTwitterGeneralInfoToDb(data['twitter_numeric'])
               data['twitter_description'] = twitterInfo.description
               data['twitter_protected'] = twitterInfo.protected
               data['twitter_profile_pic'] = twitterInfo.profile_image_url.replace("_normal", '_bigger')
               data['twitter_status'] = 2
               console.log("Tweeter: ", data)
            } 
         } else {
            data['twitter_status'] = 1

         }         
      }    

      resolve(data)

   }).then( (data) => {
      db.collection('people').doc(data.marker).set(data)   
      db.collection('people').doc(data.marker).collection('positions').doc(data['report_at']).set(
         {
            "lat":data['lat'],
            "lon":data['lon'],
         }
      )  
   }).catch( (error) => { console.log(error) })          
}

The twitterAPI functions called:

   cleanUsername: function (givenUsername) {
      return givenUsername.split('/').pop().replace('@', '').replace('#', '').split(" ").join("").split("?")[0].trim().toLowerCase()
   },  

 getTwitterGeneralInfoToDb: async function (twitter_id) {
     var endpointURL = "https://api.twitter.com/2/users/" + twitter_id
     var params = {
       "user.fields": "name,description,profile_image_url,protected"
     }

     // this is the HTTP header that adds bearer token authentication
      return new Promise( (resolve,reject) => {
         needle('get', endpointURL, params, {
          headers: {
              "User-Agent": "v2UserLookupJS",
              "authorization": `Bearer ${TWITTER_TOKEN}`
          }
        }).then( (res) => {
           console.log("result.body", res.body); 
           if (res.body['errors']) {
               if (res.body['errors'][0]['title'] == undefined) {
                  reject("Twitter API returns undefined error for :'", cleanUsername, "'")
               } else {
                  reject("Twitter API returns error:", res.body['errors'][0]['title'], res.body['errors'][0]['detail'])
               }      
            } else { 
               resolve(res.body.data)
            }
         }).catch( (error) => { console.error(error.message) })
     })
  },


  // Get unique id from Twitter user
  // Twitter API
  getTwitterIdFromUsername: async function (cleanUsername) {
    
    const endpointURL = "https://api.twitter.com/2/users/by?usernames="
    const params = {
     usernames: cleanUsername, // Edit usernames to look up     
    }


    // this is the HTTP header that adds bearer token authentication
    const res = await needle('get', endpointURL, params, {
     headers: {
         "User-Agent": "v2UserLookupJS",
         "authorization": `Bearer ${TWITTER_TOKEN}`
     }
    })
    if (res.body['errors']) {
      if (res.body['errors'][0]) {
         if (res.body['errors'][0]['title'] == undefined) {
            console.error("Twitter API returns undefined error for :'", cleanUsername, "'")
         } else {
            console.error("Twitter API returns error:", res.body['errors'][0]['title'], res.body['errors'][0]['detail'])
         }
      } else {
        console.error("Twitter API special error:", res.body)
      }
    } else {
      if (res.body['data']) {
        return res.body['data'][0].id
      } else {
        //console.log("??? Could not return ID, despite no error. See: ", res.body)
      }
      

    }
  },
Christoffer
  • 2,271
  • 3
  • 26
  • 57
  • 1
    Avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! – Bergi Dec 16 '21 at 03:08
  • Is `handleDataItem` asynchronous? Does it return a promise? – Bergi Dec 16 '21 at 03:09
  • We need to see the code for `handleDataItem()`. Since it is apparently asynchronous (updating your database) and the place where you see the problem, this is probably a big part of the overall problem. We need to help you fix that code. – jfriend00 Dec 16 '21 at 03:31
  • Still trying to follow the code from start to finish. What database are you using? Does `db.collection('people').doc(data.marker).set(data)` return a promise? – jfriend00 Dec 16 '21 at 22:31
  • I am using Google Cloud Firestore (nosql). This line of code does NOT return a promise. I have started to implement your solution with try/catch instead and it seems like it is working much better. It seems likely it will be THE solution for my issue at this time, but I need to dig much deeper into Promises as I really don't quite get it. – Christoffer Dec 17 '21 at 08:11

2 Answers2

2

You have 3 options to deal with your main issue of async methods in a loop.

  1. Instead of forEach, use map and return promises. Then use Promise.all on the returned promises to wait for them to all complete.

  2. Use a for/of loop in combination with async/await.

  3. Use a for await loop.

Steven Spungin
  • 27,002
  • 5
  • 88
  • 78
  • Thanks, I will look into those and try it. I did try a Promise.all before but never got it to work, but I will try again. – Christoffer Dec 16 '21 at 07:07
1

It sounds like there's a problem in the implementation of handleDataItem() and the promise that it returns. To help you with that, we need to see the code for that function.

You also need to clean up useData() so that it properly returns a promise that propagates both completion and errors.

And, if handleDataItem() returns a promise that is accurate, then you need to change how you do that in a loop here also.

Change from this:

function useData() {
   console.log('Starting useData()')
   return new Promise( function(resolve, reject) {
      readFromCloudFileserver()
      .then(jsonListFromCloud) => {
         jsonListFromCloud.forEach((item) => {
            handleDataItem(item)
         }
      })
      .then(() => {
         resolve() // I put resolve here because it is not until everything is finished above that this function is finished
         console.log('useData() done')
      }).catch((error) => { console.error(error.message) })
   })
}

to this:

async function useData() {
    try {
        console.log('Starting useData()')
        const jsonListFromCloud = await readFromCloudFileserver();
        for (let item of jsonListFromCloud) {
            await handleDataItem(item);
        }
        console.log('useData() done');
    } catch (error) {
        // log error and rethrow so caller gets the error
        console.error(error.message)
        throw error;
    }
}

The structural changes here are:

  1. Switch to use async/await to more easily handle the asynchronous items in a loop
  2. Remove the promise anti-pattern that wraps new Promise() around an existing promise - no need for that AND you weren't capturing or propagating rejections from readFromCloudFileServer() which is a common mistake when using that anti-pattern.
  3. rethrow the error inside your catch after logging the error so the error gets propagated back to the caller
jfriend00
  • 683,504
  • 96
  • 985
  • 979
  • Thank you so much! I have two questions regarding this solution. 1. Regarding capturing rejections from readFromCloudFileServer(). I thought that was being handled by the tailing .catch()? Is it not? – Christoffer Dec 16 '21 at 06:59
  • 2. Won't updateMyApp() need a 'return new Promise' in order to propagate to the next .then(), i.e. saveData(). When I have tried to do useData() without a 'return new Promise' it won't continue to saveData(). – Christoffer Dec 16 '21 at 07:05
  • 1
    @Christoffer - Your code shows something caledl `alterData()` before your call to `saveData()`. You don't show any code for `alterData()` so I have no idea what it is. As with your earlier question, it is VERY hard for us to offer detailed, specific advice when you only show pieces of code and we can't follow the entire code execution. I offered you specific advice on the ONE function `useData()` you showed me the full code for and showed me the context in which it was getting called. – jfriend00 Dec 16 '21 at 07:12
  • 1
    @Christoffer - `updateMyApp()` should probably start with `return loadDataToServer()` so the caller of `updateMyApp()` can get the results of that promise chain. But, you don't show any code calling `updateMyApp()` so I have no idea if that is useful. I think I've offered about as much as I can without seeing the WHOLE picture here and ALL relevant code. It's too hard to guess about the missing pieces and frankly just a waste of time to try to speculate on all the things the missing code might be doing. – jfriend00 Dec 16 '21 at 07:14
  • I am truly sorry for that as I REALLY appreciate your help. I actually thought it would be easier if I abstracted a lot of the code to make the core issue stand out more. I have added the code without the abstractions now, including other functions that are called from it. Would this make it easier to follow? – Christoffer Dec 16 '21 at 09:21
  • Re: alterData(), this was a mistake of names in my abstraction. This actually refers to useData(). I have changed that now. Sorry. – Christoffer Dec 16 '21 at 09:22
  • I have implemented the solution you mentioned in your answer and it seems to work better, but still crashes with a '(node:1) UnhandledPromiseRejectionWarning: Error: socket hang up' so I can't verify if the chained order is working as expected. – Christoffer Dec 16 '21 at 09:24