Multiple Promises - Where to resolve?

Question

I Am New To PROMISE.

I'm using nodejs promisify so I have the followings declared as promise for Redis

const { promisify } = require('util');
const getAsync = promisify(client.get).bind(client);
const hmsetAsync = promisify(client.hmset).bind(client);
const hsetAsync = promisify(client.hset).bind(client);
const incrAsync = promisify(client.incr).bind(client);
const smembersAsync = promisify(client.smembers).bind(client);
const keysAsync = promisify(client.keys).bind(client);
const sismemberAsync = promisify(client.sismember).bind(client);

I have the following function and I do not know where is the right place to put the resolve(message)

function sendMessage(senderId = '', roomId = '', text = '', type='text') {

        var message = {}
        return new Promise( function (resolve, reject) {

    if (senderId == '') {
        reject('Invalid Characters in Sender Id or Sender Id is empty.');
    }


    if (roomId == '') {
        reject('Invalid Characters in Room Id or Room Id is empty.');
    }

    if (text == '') {
        console.log('Text:' ,text);
        reject('Invalid Characters in Text or Text is empty.');
    }
            //Check if Room Exist

            keysAsync('room-'+roomId).then((value) => {

                value == '' && reject('Room '+roomId+ ' does not exist.');
                sismemberAsync('roomuser-'+roomId, senderId).then((ismember)=> {
                    ismember == 0 && reject('User ' + senderId + ' is not a member of room ' + roomId);

                    const datetime = new Date();
                    incrAsync('messageId')
                    .then((id) => {
                        //Create the message Hash
                        hmsetAsync('messsage:'+id, 'id', id, 'roomId', roomId, 'senderId', senderId, "created", datetime, "text", text);
                        //add message to Set
                        saddAsync('roommessages-'+roomId, id);

                        message = { id: id, roomId: roomId, senderId: senderId, created: datetime, text: text }
                        resolve(message) //If i place here i don't get the resolve.

                    }).catch(err => reject(err))

                }).catch(err => reject(err))

            }).catch(err => reject(err))
        })
    }

And then I tried to call the function like this

tools.sendMessage(3, 4, 'Does this work?','text').then((result)=> {
  console.log('Send Message Result => ',result);
}).catch(err => { 
  console.log(err)
});

If I were to place the resolve(message) at where it is now, the above promise doesn't resolve, and "send Message result" doesn't show at all.

If i place it further out of the chain promise, the var message returns a empty object {}

My question: Where should I place the resolve in this kind of functions where I require multiple promise calls since I need to wait for multiple redis checks?

I have many functions that requires all my redis calls to be made before I can carry on to next action.

You have already promisified all asynchronous functions why exactly you need a new Promise constructor..? — Redu, Apr 21 '18 at 13:59
i want sendMessage to be a promise so when i finally call sendMessage, it can be a promise chain? And you see my codes there are mutliple reject calls as well. — Someone Special, Apr 21 '18 at 14:00
You shouldn't be wrapping anything in a new promise. Just chain your existing promises together and return the promise from the chain. — jfriend00, Apr 21 '18 at 14:03
Before the redis codes there's actually some codes to check if senderId, text, roomId contains illegal characters and reject() them. — Someone Special, Apr 21 '18 at 14:04
Then, just return `Promise.reject(...)` for those check errors before your async operations. You never need to wrap existing promises in another promise. That is considered an anti-pattern. — jfriend00, Apr 21 '18 at 14:31

Redu · Accepted Answer · 2018-04-21T15:34:59.537

1

I might consider refactoring your code as follows. If you need to catch errors at interim .then() stages then use onrejected callbacks.

function sendMessage(senderId = '', roomId = '', text = '', type='text') {
  return keysAsync('room-'+roomId)
         .then(value => value === '' ? Promise.reject('Room '+roomId+ ' does not exist.')
                                     : sismemberAsync('roomuser-'+roomId, senderId))
         .then(ismember => ismember === 0 ? Promise.reject('User ' + senderId + ' is not a member of room ' + roomId)
                                          : incrAsync('messageId'))
         .then(id => { var datetime =new Date();
                       hmsetAsync('messsage:'+id, 'id', id, 'roomId', roomId, 'senderId', senderId, "created", datetime, "text", text);
                       saddAsync('roommessages-'+roomId, id);
                       return { id: id, roomId: roomId, senderId: senderId, created: datetime, text: text };
                     })
         .then(message => doSomethingWith(message))
         .catch(errorHandler);
}

In the above code i moved the new Date() to one stage below where it is actually used however if for some reason you need datetime at the original .then() stage as shown in your question then you may as well do as follows;

function sendMessage(senderId = '', roomId = '', text = '', type='text') {
  return keysAsync('room-'+roomId)
         .then(value => value === '' ? Promise.reject('Room '+roomId+ ' does not exist.')
                                     : sismemberAsync('roomuser-'+roomId, senderId))
         .then(ismember => ismember === 0 ? Promise.reject('User ' + senderId + ' is not a member of room ' + roomId)
                                          : Promise.all([incrAsync('messageId'), new Date()]))
         .then(([id, datetime]) => { hmsetAsync('messsage:'+id, 'id', id, 'roomId', roomId, 'senderId', senderId, "created", datetime, "text", text);
                                     saddAsync('roommessages-'+roomId, id);
                                     return { id: id, roomId: roomId, senderId: senderId, created: datetime, text: text };
                                   })
         .then(message => doSomethingWith(message))
         .catch(errorHandler);
}

edited Apr 21 '18 at 15:34

answered Apr 21 '18 at 14:14

Redu

25,060
6
56
76

will sendMessage still be a promise then? I'm trying the codes now. – Someone Special Apr 21 '18 at 14:29
@Someone Special Yes. If nothing gets rejected then `sendMessage` will return a promise to resolve with the return value of `doSomethingWith(message)`. You may also consider moving the `new Date()` to one level down. It doesn't look like needed at where it is and passing it down to the next `.then` stage by array destructuring can be eliminated. – Redu Apr 21 '18 at 14:36
I have problem at hmsetAsync now where id is being passed as a promise but it needs to be a string. – Someone Special Apr 21 '18 at 14:40
@Someone Special Oops..! Moving `new Date()` below would also fix that error i suppose. I corrected the code. – Redu Apr 21 '18 at 14:47
1

You have an invalid semicolon and some missing `)` in that second code exmple. – JLRishe Apr 21 '18 at 15:18
@JLRishe Thank you for heads up. ESLint was not active. It should be fine now. – Redu Apr 21 '18 at 15:32
I don't really understand how it's chained, and I don't understand why is there suddenly a Promise.all, but it works! Are you able to tell me why is there a Promise.all there instead of just calling incrAsync like the first chain? – Someone Special Apr 21 '18 at 15:37
@Someone Special That also caught me on wrongfoot. In fact it's a very good point which might take more than a comment to explain so [I have just entered an answer to the appropriate question about that point.](https://stackoverflow.com/a/49957460/4543207). – Redu Apr 21 '18 at 15:45
I tried to practice your solution on another function, this time Promise.Resolve doesn't seems to return the results. https://stackoverflow.com/questions/49957878/promise-resolve-returns-undefined – Someone Special Apr 21 '18 at 16:30
appreciate if you can take alook at my codes and correct me. – Someone Special Apr 21 '18 at 16:30

Multiple Promises - Where to resolve?

1 Answers1

Linked