0

I have this really weird issue where my Promise's .then is not working. The error is below:

TypeError: Cannot read property 'then' of undefined

The reason I call this weird is because I am using the same structure everywhere else and it works flawlessly. I would also like to add that the actual action it is supposed to do - does get done! So this is purely an issue with the way my Promises are "coded" - just syntactic sugar.

My code snippet is as below:

var updateAllUnreadNotifications = function(){
    userModel.aggregate([
        {$match:{"profileID": 123}},
        {$unwind: "$notifications"},
        {$match:{"notifications.read": false}},
        {$group: {_id:"$_id", notifications: {$push:"$notifications"}}}
        ], function(err, notifications){
             var notificationPromises = notifications.map(function(notification) {
                return new Promise(function(resolve){
                    userModel.findOneAndUpdate(
                        {profileID: 123, "notifications.id": notification.id},
                        {"notifications.$.read": true},
                        {safe: true},
                        function(err, result) {
                            if (err){
                               return;
                            } else if (result){
                                resolve(notification);
                            }
                        }
                    );
                });
            });

            return Promise.all(notificationPromises);
        });
};

updateAllUnreadNotifications()
.then(function(notifications){
    res.json({
        message: 'All is well'
    });
});

Furthermore, I have done a console.log on the notificationPromises and it does look like a chain of Promises. And of course, the result variable is also there so this is not an issue with the DB command.

What have I messed up? I am sure it will be a very obvious one but for the life of me, I can't isolate it.

Let me know if some more information is required.

Sean
  • 1,151
  • 3
  • 15
  • 37
  • You are not returning anything from `updateAllUnreadNotifications`. Add a `return userModel.aggregate([ ... ]);` – Saugat Nov 04 '17 at 10:31
  • Hey mate and thanks for pointing it out. I thought my "return new Promise" line will cover it but come to think of it, that is for the .map loop. Would you mind steering me to the right direction? Needless to say, a bit of a novice with JS. – Sean Nov 04 '17 at 10:33
  • @ShayanKhan Edit your question. I don't see corresponding closing `]` bracket for `userModel.aggregate([` – Prakash Sharma Nov 04 '17 at 10:35
  • It looks like you have some syntax errors, is this the exact code you are using? – Joe Lissner Nov 04 '17 at 10:36
  • I missed out the "function" line for my userModel.aggregate. Now, it is complete. My apologies. – Sean Nov 04 '17 at 10:39
  • `if (err){ return;` - if there is an error, Promise.all will ever resolve, this is bad error handling - perhaps `if(err) { reject(err); }` – Jaromanda X Nov 04 '17 at 10:39
  • @JaromandaX - I have all the error handling in place but for the purposes of this issue, I don't need extra syntax so I cleaned it up. I have run the exact code with valid data (checked in advance) and could confirm the code above demonstrates the issue. – Sean Nov 04 '17 at 10:40
  • sure, have you run the code with invalid data? this has nothing to do with your now resolved issue, just something I noticed – Jaromanda X Nov 04 '17 at 10:41
  • Thanks @JaromandaX - no I haven't as I won't be actually running it but it is not related to this issue ( and it is not resolved yet). – Sean Nov 04 '17 at 10:44
  • @SaugatAcharya - actually, the return I have in place should work, shouldn't it? – Sean Nov 04 '17 at 10:55
  • Fix the syntax issues and add a return as I said. Then try to run the code. – Saugat Nov 04 '17 at 10:56
  • @SaugatAcharya - syntax issue's fixed. Now, shouldn't the return do the trick? It should chain the promises and I run a promise.all afterwards. What am I missing here? – Sean Nov 04 '17 at 10:59
  • @ShayanKhan What library you are using for database interaction? – Prakash Sharma Nov 04 '17 at 10:59
  • What is `updateAllUnreadNotifications` returning? – Saugat Nov 04 '17 at 11:03
  • @SaugatAcharya - the line "return Promise.all(notificationPromises)" which is a chain of promises. Mate, I might be missing something obvious here so bear with me – Sean Nov 04 '17 at 11:10
  • @Prakashsharma - mongoose – Sean Nov 04 '17 at 11:11
  • Why don't you use mongoose with Promises rather than using a callback. The answer that Prakash has below should work. He wrapped userModel.aggregate with `new Promise` and that should do the trick. – Saugat Nov 04 '17 at 11:15

2 Answers2

1

You need to return promise from updateAllUnreadNotifications function but you are not returning anything from that function. That's why you are getting that error.

I would say a better way is that make a separate function for processing notification and chain the promises like this

    var updateAllUnreadNotifications = function(){
      return new Promise(function(resolve,reject){
        userModel.aggregate([
            {$match:{"profileID": 123}},
            {$unwind: "$notifications"},
            {$match:{"notifications.read": false}},
            {$group: {_id:"$_id", notifications: {$push:"$notifications"}}}
            ], function(err, notifications){
                 if(err){ reject(err)}
                 // If got notifications then resolve
                 // notifications will be available to next then function
                 resolve(notifications)
             })
         })
      }

      var processNotifications = function(notifications) {
         var notificationPromises = notifications.map(function(notification) {
              return new Promise(function(resolve){
                 userModel.findOneAndUpdate(
                    {profileID: 123, "notifications.id": notification.id},
                     {"notifications.$.read": true},
                            {safe: true},
                            function(err, result) {
                                if (err){
                                   return;
                                } else if (result){
                                    resolve(notification);
                                }
                            }
                        );
                    });
                });

         return Promise.all(notificationPromises);
       };

    // Chain one extra then here to process notifications
    updateAllUnreadNotifications().then(processNotifications)
    .then(function(notifications){
        res.json({
            message: 'All is well'
        });
    });
Prakash Sharma
  • 15,542
  • 6
  • 30
  • 37
1

You return from the userModel.aggregate callback - which cannot work - instead of from the updateAllUnreadNotifications function. It does return undefined, on which you try to invoke the then method. You will need to promisify that as well:

function aggregateAsync(x) {
    return new Promise((resolve, reject) => {
        userModel.aggregate(x, (err, res) => {
            if (err) reject(err);
            else resolve(res);
        });
    });
}
function findOneAndUpdateAsync(x, y, z) {
    return new Promise((resolve, reject) => {
        userModel.findOneAndUpdate(x, y, z, (err, res) => {
            if (err) reject(err);
            else resolve(res);
        });
    });
}

With those, you can do

function updateAllUnreadNotifications() {
    return aggregateAsync([
        {$match:{"profileID": 123}},
        {$unwind: "$notifications"},
        {$match:{"notifications.read": false}},
        {$group: {_id:"$_id", notifications: {$push:"$notifications"}}}
    ]).then(notifications => {
        var notificationPromises = notifications.map(notification => {
            return findOneAndUpdateAsync(
                {profileID: 123, "notifications.id": notification.id},
                {"notifications.$.read": true},
                {safe: true}
            );
        });
        return Promise.all(notificationPromises);
    });
}

updateAllUnreadNotifications().then(notifications => {
    res.json({
       message: 'All is well'
    });
});
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • Thanks for the answer Bergi. I will try this as well (as you can see, the other answer worked for me). But as a learning exercise, I will go through your code so I can understand Promises a little more. Every time I think I have nailed Promises, I sorta get lost. – Sean Nov 07 '17 at 14:50