Node.js for loop using previous values?

Question

Currently, I am trying to get the md5 of every value in array. Essentially, I loop over every value and then hash it, as such.

var crypto = require('crypto');
  function userHash(userIDstring) {
    return crypto.createHash('md5').update(userIDstring).digest('hex');
  }

  for (var userID in watching) {
    refPromises.push(admin.database().ref('notifications/'+ userID).once('value', (snapshot) => {
        if (snapshot.exists()) {
        const userHashString =  userHash(userID)
        console.log(userHashString.toUpperCase() + "this is the hashed string")
        if (userHashString.toUpperCase() === poster){
            return console.log("this is the poster")
        }
        else {
            ..
        }
    }
    else {
        return null
    }
      })
    )}

However, this leads to two problems. The first is that I am receiving the error warning "Don't make functions within a loop". The second problem is that the hashes are all returning the same. Even though every userID is unique, the userHashString is printing out the same value for every user in the console log, as if it is just using the first userID, getting the hash for it, and then printing it out every time.

Update LATEST :

exports.sendNotificationForPost = functions.firestore
    .document('posts/{posts}').onCreate((snap, context) => {
      const value = snap.data()
      const watching = value.watchedBy
      const poster = value.poster
      const postContentNotification = value.post
      const refPromises = []
      var crypto = require('crypto');
      function userHash(userIDstring) {
        return crypto.createHash('md5').update(userIDstring).digest('hex');
      }

      for (let userID in watching) {
        refPromises.push(admin.database().ref('notifications/'+ userID).once('value', (snapshot) => {
            if (snapshot.exists()) {
            const userHashString =  userHash(userID)

            if (userHashString.toUpperCase() === poster){
                return null
            }
            else {

                const payload = {
                    notification: {
                        title: "Someone posted something!",
                        body: postContentNotification,
                        sound: 'default'
                    }
                };

                return admin.messaging().sendToDevice(snapshot.val(), payload)
            }
        }
        else {
            return null
        }
          })
        )}
        return Promise.all(refPromises);
    });

Answer 1

These are not two problems: the warning you get is trying to help you solve the second problem you noticed.

And the problem is: in Javascript, only functions create separate scopes - every function you define inside a loop - uses the same scope. And that means they don't get their own copies of the relevant loop variables, they share a single reference (which, by the time the first promise is resolved, will be equal to the last element of the array).

Just replace for with .forEach.

Answer 2

You have a couple issues going on here. First, you have a non-blocking asynchronous operation inside a loop. You need to fully understand what that means. Your loop runs to completion starting a bunch of non-blocking, asynchronous operations. Then, when the loop finished, one by one your asynchronous operations finish. That is why your loop variable userID is sitting on the wrong value. It's on the terminal value when all your async callbacks get called.

You can see a discussion of the loop variable issue here with several options for addressing that:

Asynchronous Process inside a javascript for loop

Second, you also need a way to know when all your asynchronous operations are done. It's kind of like you sent off 20 carrier pigeons with no idea when they will all bring you back some message (in any random order), so you need a way to know when all of them have come back.

To know when all your async operations are done, there are a bunch of different approaches. The "modern design" and the future of the Javascript language would be to use promises to represent your asynchronous operations and to use Promise.all() to track them, keep the results in order, notify you when they are all done and propagate any error that might occur.

---

Here's a cleaned-up version of your code:

const crypto = require('crypto');

exports.sendNotificationForPost = functions.firestore.document('posts/{posts}').onCreate((snap, context) => {
    const value = snap.data();
    const watching = value.watchedBy;
    const poster = value.poster;
    const postContentNotification = value.post;

    function userHash(userIDstring) {
        return crypto.createHash('md5').update(userIDstring).digest('hex');
    }

    return Promise.all(Object.keys(watching).map(userID => {
        return admin.database().ref('notifications/' + userID).once('value').then(snapshot => {
            if (snapshot.exists()) {
                const userHashString = userHash(userID);

                if (userHashString.toUpperCase() === poster) {
                    // user is same as poster, don't send to them
                    return {response: null, user: userID, poster: true};
                } else {
                    const payload = {
                        notification: {
                            title: "Someone posted something!",
                            body: postContentNotification,
                            sound: 'default'
                        }
                    };
                    return admin.messaging().sendToDevice(snapshot.val(), payload).then(response => {
                        return {response, user: userID};
                    }).catch(err => {
                        console.log("err in sendToDevice", err);
                        // if you want further processing to stop if there's a sendToDevice error, then
                        // uncomment the throw err line and remove the lines after it.  
                        // Otherwise, the error is logged and returned, but then ignored 
                        // so other processing continues

                        // throw err

                        // when return value is an object with err property, caller can see
                        // that that particular sendToDevice failed, can see the userID and the error
                        return {err, user: userID};    
                    });
                }
            } else {
                return {response: null, user: userID};
            }
        });
    }));
});

Changes:

1. Move require() out of the loop. No reason to call it multiple times.
2. Use .map() to collect the array of promises for Promise.all().
3. Use Object.keys() to get an array of userIDs from the object keys so we can then use .map() on it.
4. Use .then() with .once().
5. Log sendToDevice() error.
6. Use Promise.all() to track when all the promises are done
7. Make sure all promise return paths return an object with some common properties so the caller can get a full look at what happened for each user