What are the down sides to wrapping promises in an object that resolves them?

Question

I'm working on a new framework of microservices built in Node 8 and trying to simplify some of the logic required for passing Promises around between services.

I have a function I import in each service called StandardPromise which you can pass a Promise to. StandardPromise will call .then() on the promise and place the result in an object. If the promise was resolved it will be placed in the data attribute, if was rejected or threw an error then that will go in the err attribute.

The result of the above is that when a service receives a standardized promise by awaiting a call to another service, it can just check if there's anything in err and move forward with data if err is empty. This flow is significantly simpler than having .then() and .catch() blocks in every function.

I'm pretty happy with how it's turning out, and it seems to be working great, but since I haven't seen many examples of this kind of flow I want to know if there's something I'm missing that makes this a terrible idea or an antipattern or anything like that.

Here's a simplified, somewhat pseudocode example:

Service1:

const sp = require('./standardPromise');
const rp = require('request-promise-native');

function ex() {
    // Wrap the Promise returned from rp as a "standardPromise"
    return sp(rp.get({url: 'https://example.com'}));
}

Service2:

const Service1 = require('./Service1');

async function ex2() {
    var res = await Service1.ex();
    if (res.err) {
        // Do error stuff
        console.error(res.err);
        return;
    }
    // Here we know res.data is our clean data
    // Do whatever with res.data
    return res.data;
}

standardPromise:

module.exports = function(promise) {
    try {
        return promise.then((data) => {
            return {err: undefined, data: data};
        }).catch((err) => {
            return Promise.resolve({err: err, data: undefined});
        });
    } catch(err) {
        console.error('promise_resolution_error', err);
        return Promise.resolve({err: err, data: undefined});
    }
}

This isn't really clear without some code. It's hard to understand why the caller of the service doesn't have to deal with a different promise created when the service calls `then()` on the the original one. In other words, how does the caller access the `data` object? — Mark, Oct 07 '18 at 15:59
so in every place there is `if(response.err) ... else ...` right? — skyboyer, Oct 07 '18 at 16:16
@skyboyer Right. Essentially it allows each service to deal with errors just by checking the `res.err` property returned by the preceding service rather than all needing `.then() {} ... .catch() {}` blocks. And it also deals with wrapping the promise resolution in a `try {} catch() {}` so that unexpected non-promises are handled gracefully without needing to add that complexity to every service. — guyot, Oct 07 '18 at 16:24
Why not use the native Promise functionality of `resolve` and `reject` (or the `async/await` pattern of `try/catch`) instead of rolling your own and confusing later users of your code? — Heretic Monkey, Oct 07 '18 at 16:24
@HereticMonkey Well the promise that is being wrapped will be `resolve()`ing or `reject()`ing, so that pattern is being used. I'm using `async / await` to get those values, but including `try/catch` blocks in every service is a lot messier. This simplifies the error flow that individual services need to implement. Regarding confusing users, this is all going to be a well-documented internal pattern. I think the simplified error logic is really nice so new developers just need to learn what the standardPromise wrapper is doing, which is pretty simple. — guyot, Oct 07 '18 at 16:28
Your "StandardPromise" never rejects though; it only resolves, so you're not actually using that pattern. Any library or framework that you might want to use later on, that assumes the default pattern, will not work with this code. That might be a risk you're willing to take on, in which case have at it. I just want to warn you, and others thinking of using code like this, that you are taking on (unwarranted, in my opinion) risk. — Heretic Monkey, Oct 07 '18 at 16:33
@HereticMonkey Oh sorry I see what you're saying. Yes it does not reject, but that is the point of it. If the wrapped promise is rejected then StandardPromise picks that up in the `.catch()` and passes it out in the `err` property. Because it always resolves, the service receiving it only has to check if there's a `.err` to know if the Promise was rejected or not, which greatly simplifies the error flow. — guyot, Oct 07 '18 at 16:38
@HereticMonkey And yes you're very right that this means any packages which use the default pattern will be incompatible. Thanks for pointing that out though, and anyone reading this should note that as well. Downsides like that are what I'm trying to determine with this post. — guyot, Oct 07 '18 at 16:38
...and in case you have few requests chaining for native Promise it will be `request1.then(request2).then(request3).catch(somethingWentWrong)` or just list of `await`. But with that `StandardPromise` you have to check for `.err` on every request. — skyboyer, Oct 07 '18 at 16:41
@skyboyer Good call, yeah Promise chains are another downside, that'll be good for me to keep in mind. In my case most of these are super simple services which are just retrieving a value and returning, but this will definitely make complicated chaining functionality more difficult so I'll have to think through that case. Thanks! — guyot, Oct 07 '18 at 16:44
You prevent yourself from using half of `Promise.all`, which could be useful even - particularly! - in your micro-services logic. Instead, you'll _always_ need to loop over all results for an hypothetical error, because `.all` would _never_ reject. Seems like a big downside to me, performance-wise: if successful, you still need to uselessly walk results for errors to assert success; if unsuccessful, not only would you walk the results, but you'll have waited for all promises whereas `Promise.all` rejects as soon as one promise fails. — Stock Overflaw, Oct 07 '18 at 17:07
@StockOverflaw Excellent point, that one I had not though of yet. Thanks I will consider. — guyot, Oct 07 '18 at 17:13

score 2 · Answer 1 · answered Oct 07 '18 at 17:25

It can just check if there's anything in err and move forward with data if err is empty. This flow is significantly simpler than having .then() and .catch() blocks in every function.

No, this is much more complicated, as you always have to check for your err. The point of promises is to not have .catch() blocks in every function, as most functions do not deal with errors. This is a significant advantage over the old nodeback pattern.

You would drop your standardPromise stuff and just write

// Service1:
const rp = require('request-promise-native');

function ex() {
    return rp.get({url: 'https://example.com'});
}

// Service2:
const Service1 = require('./Service1');
async function ex2() {
    try {
        var data = await Service1.ex();
    } catch(err) {
        // Do error stuff
        console.error(err);
        return;
    }
    // Here we know data is our clean data
    // Do whatever with data
    return data;
}

or actually simpler with then for handling errors:

// Service2:
const Service1 = require('./Service1');
function ex2() {
    return Service1.ex().then(data => {
        // Here we know data is our clean data
        // Do whatever with data
        return data;
    }, err => {
        // Do error stuff
        console.error(err);
    });
}

With the advent of the newly minted async stack traces, I'd go as far as going `async function ex() { return await rp.get(...); }` — Madara's Ghost, Oct 07 '18 at 17:28
@MadaraUchiha [I don't like `return await`](https://stackoverflow.com/a/43985067/1048572). Is that really necessary to get async stack traces? I think they work just as fine with normal functions in the async stack. — Bergi, Oct 07 '18 at 17:31
I think I heard Benji mention it once or twice... Either way, it's just "semantically" different, in one you return a promise and rely on auto-unwrapping to make it work, and on the other the awaiting is explicit. To each their own. — Madara's Ghost, Oct 07 '18 at 17:45

What are the down sides to wrapping promises in an object that resolves them?

1 Answers1