0

I have the following code that I use to signup users.

const pgp = require('pg-promise')();
const crypto = require('./crypto.js');

const db = pgp(connection);

const querySelect = (text, params) => {  
  const s = pgp.as.format(text, params);  
  return db.any(s)
  .then(data => {    
    return data;    
  })
  .catch(error => {
    return error;
  });
}

const queryInsert = (text, params) => {  
  const s = pgp.as.format(text, params);  
  return db.one(s)
  .then(data => {    
    return data;    
  })
  .catch(error => {
    return error;
  });
}

const signup = (user) => {
  return new Promise((resolved, rejeted)=>{    
    return querySelect('select username from user WHERE username = $1', [user.username])      
    .then(res => {
      if (res.length == 0) {              
        return true;
      }      
      return resolved(false);  //SHOULD STOP + RETURN HERE                                 
    })
    .then(hashPasswordRes=>{      
      return crypto.hashPassword(user.password);
    })
    .then(queryRes=>{      
      if (queryRes) {
        return queryInsert('insert into user (username,password) values ($1,$2) RETURNING id', 
        [user.username, user.password]);      
      } else {            
        return rejeted('error while signup');
      }
    })
    .then(res=>{
      resolved(true);          
    })
    .catch(error => {
        return rejeted('error while signup');
    });// catch
  }) //promise
};//signup

exports.signup = signup;

This is basically the whole file. Pretty simple I guess.

The problem is that in the point where I comment SHOULD STOP + RETURN HERE it does return false (so that means that the is already an existing username like the one inserted) but the execution never stops, so the user ends up saved in the database (even though the return resolved(false); is executed).

What am I doing wrong here? This used to work when I had just the pg module in my code. When I used pg-promise, it started having this issue.

Frankly, I dont have a clue why the "return" doesnt also stop the function execution. Please advice

(node 8.11.1, express 4.16.3, pg 7.4.2, pg-promise 8.4.4)

Thanks

EDIT

for the record, here is the same file without pg-promise , that works just fine. The vars names change, but you can see the logic. I tried to use the same logic with pg-promise

  const crypto = require('./crypto.js');
  const {Client} = require('pg');


const getuser = (mail, client) => {
  return new Promise((resolved, rejeted)=>{
    return client.query('select mail from user WHERE mail = $1',[mail])
    .then(res => {
      resolved(res.rows);
     })
     .catch(e => {
        rejeted(e);
        client.end();
     }); // catch
  })//promise
} //getUser


const signup = (user) => {
  return new Promise((resolved, rejeted)=>{
    client.connect().then(() => {
        getuser(user.email, client) 
        .then(getUserRes => {
          if (getUserRes.length==0) {
            return true;
          }
          client.end();
          return resolved(false);
        })
        .then(hashPasswordRes=>{
            return crypto.hashPassword(user.password);
        })
        .then(queryRes=>{
          if (queryRes) {
            const nowtime = new Date();
            return client.query('insert into user(mail, password) values ($1,$2)',
            [user.email, queryRes])
          } else {
            client.end();
            return rejeted('some error');
          }
        })
        .then(res=>{
          resolved(true);
          client.end();
        })
    }) // client.connect then
    .catch(error => {
      rejeted('some error');
      client.end();
    });// catch
  }) //promise
};//signup


exports.signup = signup;
slevin
  • 4,166
  • 20
  • 69
  • 129
  • Your promise-related stuff looks quite invalid. Your return things from the callback where the return value is irrelevant. That's generic promise misuse. Also why are you trying to re-implement query methods of pg-promise? This doesn't make sense. The library already has all the methods that you can execute directly. And you do not need any extra promises that you are creating in your code. Your example has so much redundant code. – vitaly-t Jun 04 '18 at 17:56
  • @vitaly-t No offence, I am sorry, but I fail to see what is wrong. I have a promise in a function. It starts by calling a function that returns some data. Then, it continues returning from one then to the other. I am not trying to re-implement. I just have the pg queries outside of the signup function and I move through the then, according to the outcomes of the various functions called. At least that was my initial plan. You can also check the version of my code that worked, in the edited part of my question. – slevin Jun 04 '18 at 20:47
  • 1
    @vitaly-t Hi again vitaly, I think I have a [solution](https://codereview.stackexchange.com/questions/195973/have-a-then-chain-inside-a-promise-and-skip-that-chain) for the problem, I dont know if its perfect, but I finally see a lot of issues in my code. Anyway, thanks for your time and all the advice. – slevin Jun 06 '18 at 17:02
  • have a look at [pg-promise-demo](https://github.com/vitaly-t/pg-promise-demo), it is a good starting point ;) – vitaly-t Jun 06 '18 at 17:19
  • @vitaly-t yes, this is a great resource, showing a smart use of the whole pg-promise, but I cannot find a solution that solves my particular original problem of the chain of `then`s. – slevin Jun 07 '18 at 18:34

1 Answers1

1

The return ends the current function that is running, which is not the promise chain. So this part:

return resolved(false);  //SHOULD STOP + RETURN HERE

Ends the current function, which is:

res => {
      if (res.length == 0) {              
        return true;
      }      
      return resolved(false);  //SHOULD STOP + RETURN HERE                                 
    }

Notices that the arrow declares a function.

If you wanna stop a chain you can do something like this

Promise.resolve(res)
.then(res => {
    if (shouldKeepGoing(res)) {
        return allTheOtherPromises(res)
    }
    return true
})

const allTheOtherPromises = res => {
    // Here you do all the promises you were doing
}
Denis
  • 2,030
  • 1
  • 12
  • 15
  • Check my original question, the EDIT part, this is when my code used to work. I tried to use the same logic, I dont get what went wrong. Thanks for your answer. I get the first part, explaining what is wrong , but I am still confused by how I should re-write my code, your syntax is different. Anyway, I will check it tomorrow and let you know, its getting pretty late here now and I cannot think clear. I will test again tomorrow. – slevin Jun 04 '18 at 20:53
  • Hi again, thanks for the advice. I think I got a solution, check [here](https://codereview.stackexchange.com/questions/195973/have-a-then-chain-inside-a-promise-and-skip-that-chain) if you like. Thank you very much. – slevin Jun 06 '18 at 17:00
  • Glad to hear it! – Denis Jun 06 '18 at 21:29