0

I'm stuck on what the best way to create a wrapper callback function in node/express that reuses the same parameters.

The thing is that the verification requires the same parameters the callback does. Is there a way to simplify this? I've looked for this on stack and google, but couldn't find an answer. I don't want to write req, res, next twice in the actual call. I understand the first req, res, next are being funneled into the callback, but it still feels weird to write it like this. It feels like there's definitely a better approach, but I just don't know what that is.

Here's the situation:

function verifyCaptcha(req, res, next, callback) {

    let captchaResponse = req.body.captchaResponse;

    let captchaSecret = "it's a secret";

    let captchaURL = "https://www.google.com/recaptcha/api/siteverify?"
    + "secret=" + encodeURIComponent(captchaSecret) + "&"
    + "response=" + encodeURIComponent(captchaResponse) + "&"
    + "remoteip" + encodeURIComponent(req.connection.remoteAddress);

    // request is ASYNC, that's why I need the callback
    request(captchaURL, function(error, response, body) {
        callback(req, res, next);
        return;
    });
};


router.post('/login', function(req, res, next) {

    // example call INSIDE a route (here's the problem b/c params are repeated in the call
    verifyCaptcha(req, res, next, function(req, res, next){
        // do stuff
    });

};
jonrsharpe
  • 115,751
  • 26
  • 228
  • 437
Viet Tran
  • 23
  • 1
  • 4
  • I don't see any problem with the current code. I guess you are stuck in the so called "can we make it more concise" dilemma, which in most cases is just a waste of time. – Ankur Aug 25 '18 at 10:02

2 Answers2

1

Promises are supposed to avoid callback hell. All popular callback-based libraries have promisified counterparts, it's request-promise for request. This can be written in sync-like manner with async..await:

const request = require('request-promise');

async function verifyCaptcha(req, res) {
    let captchaResponse = req.body.captchaResponse;

    let captchaSecret = "it's a secret";

    let captchaURL = "https://www.google.com/recaptcha/api/siteverify?"
    + "secret=" + encodeURIComponent(captchaSecret) + "&"
    + "response=" + encodeURIComponent(captchaResponse) + "&"
    + "remoteip" + encodeURIComponent(req.connection.remoteAddress);

    const result = await request(captchaURL);
    ...
};


router.post('/login', async function(req, res, next) {
    try {
        await verifyCaptcha(req, res);
    } catch (err) {
        next(err);
    }
};

As explained in this question, Express doesn't support promises natively, all rejections should be handled by a developer, async middleware/handler function body should be wrapped with try..catch.

Estus Flask
  • 206,104
  • 70
  • 425
  • 565
0

According to the express documents, you can chain middleware by simply adding it to the route call.

http://expressjs.com/en/4x/api.html#app.use

which allows you to do this:

function verifyCaptcha(req, res, next) {
    let captchaUrl = ...

    request(captchaUrl, function(error, response, body) {
        if (error) res.sendStatus(403)
        next()
    }
}

router.post('/login', verifyCaptcha, function(req, res, next) {
    // this will only run if verifyCaptcha calls next()
}

The end result is a lot more readable in practice.

Viet Tran
  • 23
  • 1
  • 4
  • This wasn't obvious from the original code that verifyCaptcha is in fact a middleware because there is no `res.sendStatus(403)` there. – Estus Flask Aug 25 '18 at 13:01
  • @estus I wasn't completely sure. I'm a bit new to node/express and I wasn't really sure what 'middleware' was until another redditor pointed it out to me. As far as I'm concerned however, your approach also works. There may cases where I don't want to wrap the entire execution with the verification and your approach via promises makes more sense while still providing readable code. My post originally started as a reddit post, so I wanted to keep the code concise, the full code has a lot of error checks and I wanted to keep it short. – Viet Tran Aug 25 '18 at 21:45