6
\$\begingroup\$

I'm using a Promise chain to write some data to a file, execute a command on it, inspect its results, and send the results, using Express.js. The thing about my function is that I need to be able to exit early from multiple conditions.

The following code comes from an Express.js handler. my-command is a command that takes one input file, and outputs either resultWithBadName.txt, when the output is only one "page," and result-{id}.txt, when the output has multiple pages.

var cp = require('child_process'),
    Promise = require('bluebird');

var results,
    id = req.param.id; // from user

promisifiedWriteFile('src.txt', 'foobar' /* real content from user */).catch(function (err) {
    return Promise.reject({ text: 'Cannot write file', err: err });
}).then(function() {
    return exec(
        'my-command src.txt'
    ).catch(function (err) {
        // Considered a user error.
        res.send({
            error: err.stderr,
            id: id,
            pages: 0
        });
        return Promise.reject('DONE');     // UGLY
    });
}).then(function (ret) {
    results = ret;
    // Check if output only has one page
    return promisifiedStat(
        'resultWithBadName.txt'
    ).catch(function () {
        // Nope, has multiple pages. Coutn how many pages there are.
        return countPagesInOutput(id).then(function (pages) {
            res.send({
                output: results.stderr,
                id: id,
                pages: pages
            });
            return Promise.reject('DONE'); // UGLY
        });
    });
}).then(function () {
    // Yep, only has one page. Let's make the result have standard naming scheme.
    return promisifiedRename(
        'resultWithBadName.txt',
        'result' + id + '.txt'
    ).catch(function (err) {
        return Promise.reject({ text: 'file rename failed', err: err });
    });
}).then(function () {
    res.send({
        output: results.stderr,
        id: id,
        pages: 1
    });
}).catch(function (err) {
    if (err === 'DONE') return;         // UGLY
    res.status(500).send('Internal server error: ' +
        (err.text || err.message || '')
    );
    console.error(err.err || err);
}).catch(console.error);

function exec() {
    var args = [].slice.apply(arguments);
    return new Promise(function (fulfill, reject) {
        args.push(function (err, stdout, stderr) {
            if (err) {
                err.stderr = stderr;
                return reject(err);
            }
            fulfill({
                stdout: stdout,
                stderr: stderr
            });
        });
        cp.exec.apply(cp, args);
    });
};

I have read somewhere that one can chain the thens inside a onFulfilled handler, but that adds even more pyramids to my code, which Promises are supposed to prevent…

Now my question is, is there a way to remove the ugly 'DONE' hack?

\$\endgroup\$
1
  • \$\begingroup\$ Hi Timothy. You asked a lot of time ago, but I have the same problem now. You can see my explanation here. I am not an expert, but it makes sense and maybe it will help you. The bottom line is that if you want to skip a chain of then,you have to throw an error. Also, in your code, you can fix the series of this and have only one catch, by checking this . The videos in my questions may help you further. \$\endgroup\$
    – slevin
    Jun 6, 2018 at 17:20

1 Answer 1

1
\$\begingroup\$

I realize this question is old now, but it's intriguing and still getting a number of views, so I though I would give it a shot.

Now days there's a very easy solution to this: Just use async/await syntax instead so that you can use normal flow-control statements to get the job done. However, I'll write this under the assumption that this task needs to be done in legacy es5.

Lets take a simplified example. A lot of the links in your promise chain tended to have a structure such as this:

firstTask()
    // before...
    .then(function () {
        return asyncTask()
            .catch(function (err) {
                // handle catch
                return Promise.reject('DONE'); // This gets caught at the end of the promise-chain and ignored
            });
    })
    // after...

You're right in assuming that it's not such a great idea to be rejecting with a sentinel like that in order to skip over future .then()s. The proper solution is to restructure this in order to make it so there's nothing that needs to be skipped over. The following would have the same effect, but without needing to reject with a sentinel.

firstTask()
    // before...
    .then(asyncTask)
    .then(
        function () {
            // after ...
        },
        function () { /* handle catch */ }
    )

While this does fix the issue, you'll notice an important problem: The // after ... part is now nested a couple levels deep. Each time we need this skip-to-the-end behavior, we will have to add even further nesting, and that's just gross 🤮️ - we just got ourselves right back into the callback hell that promises were designed to prevent. This is an unfortunate limitation to promises, however, even callback hell can be tamed with some good coding practices. If we split up the logic into multiple functions, then we can avoid having too much nesting happening in one place.

firstTask()
    // before...
    .then(asyncTask)
    .then(
        doAfter,
        function () { /* handle catch */ }
    )

function doAfter() {
    // after ...
}

And now we have a recipe to fix up this code! Below I have an (untested) version of the original code that's been fixed up so it doesn't need to break out of a promise chain anymore. I did make one change that simplified things a bit: I took out the places where you would catch an error, then re-reject it like this Promise.reject({ text: 'file rename failed', err: err }), which you later re-caught, logged the error, and returned the text to the user. The end-user doesn't need to know what specific internal detail went wrong, simply giving back "Internal Server Error" should be good enough.

var id = req.param.id;

promisifiedWriteFile('src.txt', 'foobar')
    .then(function () { return exec('my-command src.txt') })
    .then(onExecute, onExecuteError)
    .then(function (response) { res.send(response) })
    .catch(function (err) {
        res.status(500).send('Internal server error');
        console.error(err.message);
        console.error(err.stack);
    });

function onExecute (ret) {
    var fileHasBadName;
    return isFileNameBad()
        .then(function (fileHasBadName_) {
            fileHasBadName = fileHasBadName_;
            if (fileHasBadName) return fixBadFileName(id);
        })
        .then(function () {
            return fileHasBadName ? 1 : countPagesInOutput(id);
        })
        .then(function (pages) {
            return { output: ret.stderr, id: id, pages: pages };
        });
}

function onExecuteError (err) {
    return { error: err.stderr, id: id, pages: 0 };
}

function isFileNameBad() {
  return promisifiedStat('resultWithBadName.txt')
    .then(function () { return true }, function () { return false })
}

function fixBadFileName(id) {
    return promisifiedRename('resultWithBadName.txt', 'result' + id + '.txt')
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.