Unhandled promise rejection in Node.js

3
votes

I'm trying to make a DELETE call and I'm implementing the function below. I understand that in a promise, there needs to be a "resolve" and a "reject" state, but I'm getting an unhandled promise rejection error:

UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): [object Object]

I don't really like using conditional statements inside a promise because it gets messy, but what I'm trying to do here is to check if the organization is verified, and if it is, a delete operation should not occur and will reject. 

function deleteOrg(id) {
    return new Promise((resolve, reject) => {
        // A helper function that returns an 'org' object
        findById(id)
        .then((orgObject) => {
            if (orgObject.verified_at !== null) {
                throw new Error(422, 'Unable to delete organization')
            }

            //Organization is not verified, so proceed to delete
            new Organization().where({'id': id}).destroy()
            .then(() => {
                return resolve() //return 200 upon deletion
            })
            .catch((err) => {
                return reject(new Error(500, 'Unable to delete organization'))
            })
        })
        .catch((err) => {
            const m = `Unable to delete organization: ${err.message}`
            return reject(new Error(500, m))
        })
    })
}

I'm pretty sure I'm handling the rejection inside the if wrong.

2
javascriptnode.jsecmascript-6promise
Avoid the Promise constructor antipattern! – Jaromanda X
As @JaromandaX said! Since findById returns a Promise why do you even need the Promise constructor? You could just then it's result. – Balázs Édes
@JaromandaX Thank you for sharing my links :-) – Bergi
Is there any kind of stack trace from the unhandled rejection? – Bergi
@Bergi - I've seen it so often I had to :p – Jaromanda X

2 Answers

1
votes

as findById and .destroy return Promises, there is no need for the Promsie constructor

Your code is then simplified to

function deleteOrg(id) {
    return findById(id)
    .then((orgObject) => {
        if (orgObject.verified_at !== null) {
            throw new Error(422, 'Unable to delete organization')
        }

        //Organization is not verified, so proceed to delete
        return new Organization().where({'id': id}).destroy()
        .catch((err) => {
            throw (new Error(500, `Unable to delete organization: ${err.message}`));
        });
    });    
}
1
votes

Creating promises inside promise constructors is a known anti-pattern. Try modularizing your promises into separate functions instead:

function deleteOrg(id) {

  const verifyOrg = (orgObject) => {
    if (orgObject.verified_at !== null) {
      throw new Error(422, 'Unable to delete organization')
    }
  };

  const destroyOrg = () => new Organization().where({
    'id': id
  }).destroy();

  return findById(id)
    .then(verifyOrg)
    .then(destroyOrg);
}

Where you can let the errors propagate through the promise chain and handle them outside:

deleteOrg(id)
  .catch((err) => {
    const m = `Unable to delete organization: ${err.message}`;
    // ...
  });