javascript
javascript copied to clipboard
airbnb
Best practices for promises?
First of all, great job on this guide, I agree with it almost 100%. All of this should really be common knowledge, but I sure as hell didn't know most of these things when I started, so kudos for writing it all down.
Now, since promises have been standardized and a lot of people have started using them, it would be nice to have a section on how to keep promise-based code readable. For instance, when chaining multiple promises together, there are at least two alternative ways to do it:
Option 1: anonymous functions
return doSomething(foo).then(function(result) {
// ...
}).then(function(result2) {
// ...
}).then(function(result3) {
// ...
}).catch(function(error) {
// ...
}).finally(function() {
// ...
});
Option 2: named functions
function doThis(result) {
// ...
}
function doThat(result) {
// ...
}
function handleError(error) {
// ...
}
function done() {
// ...
}
return doSomething(foo)
.then(doThis)
.then(doThat)
.catch(handleError)
.finally(done);
The second way, while being somewhat more verbose, seems to be preferable, as the chain of tasks that the promise result travels through becomes more obvious.
I did an internal poll of our developers, and so far, option 2 seems to be ahead, although I have not heard from everyone yet. How does AirBnB deal with this, and what are your opinions?
Hi @PandaWhisperer, thanks for opening this Issue. Promises are a great place to have conventions in place for clarity.
I'm going to leave this open for a bit to give time for the community to participate in the discussion.
Will follow up on this after a week or so.
Option 2 is definitely more popular in node where we've got module.exports and don't have to worry about those functions leaking globally. I do this on the front-end as well and a lot of people do, but a lot of times, new programmers or non-JS programming who are writing JS, use anonymous functions - and a lot of them write the same exact anonymous functions a dozen times. I.e:
return doSomething(foo).then(function(result) {
$('#mything').html(result);
}).then(function(result2) {
$('#mything2').html(result);
}).catch(function(error) {
console.log("There was an error!", error);
});
return doSomethingElse(foo).then(function(result) {
$('#mything').html(result);
}).then(function(result2) {
$('#mything2').html(result);
}).catch(function(error) {
console.log("There was an error!", error);
});
This is a really minimal example, but what I'm getting at is that we should push to recommend option 2 for best practices, particularly calling out at least these two major values I can think of:
- ability to reuse named functions, saving memory and centralizing locations that will need to change
- named functions provide much more useful stack traces in error messages
Good work on this!! I'm looking forward to seeing what people have to say about it. It could also be useful to recommend promise implementations based on various criteria:
- most popular libs
- most lightweight
- most package manager support
Here's another situation I just stumbled upon while working with some Promise-based code:
getPromise().then(function(result) {
if (!checkResult(result)) {
throw new Error("invalid result!");
} else {
return doSomething(result);
}
}).catch(handleError);
Obviously, this code is a bit contrived, but bear with me here, the point should become obvious in a second. The above code is completely equivalent to this:
getPromise().then(function(result) {
if (!checkResult(result)) {
return Promise.reject(new Error("invalid result!"));
} else {
return Promise.resolve(process(result));
}
}).catch(handleError);
Of course, .then(), by design, ALWAYS returns a promise, so making this explicit does not add anything. You'd think that would make it obvious which version is preferable – but the fact that the code I have in front of me right now uses the 2nd version shows otherwise.
I'm not a fan of using named functions if the only reason is for the names to show up in stack traces. Part of the appeal of promises (and, more generally, functional programming) is to compose complex behaviors from simple ones, and anonymous functions are an important part of making that easier to do.
Of course, if we have a complex behavior that can be reused, that's a different story. But for, say, up to 3 lines of code (a completely arbitrary threshold), anonymous functions are fine.
The issue of handling typical error callbacks, though, suggests those functions should be lifted by your promise library. That is, I don't think you should typically need to map errors to reject and results to resolve.
If you're not using some standard callback form (for which lifting functions would be available), one question is: why not? And another is: does your particular callback style come up often enough to write your own standard lifting function.
I definitely +1 @dyoder on that. Sure it helps debugging but named functions require the developer to 1/ find a good name 2/ keep that name consistent over time with the rest of the code base.
In ES6 in some cases it will become even more elegant / simple :
getPromise().then((response) => response.body)
Instead of
getPromise().then(function readBody(response) {
return response.body;
})
About best practices, seems like you all put the .then on the same line instead of a new line...
What do you think of
getPromise()
.then(function(response) {
return response.body;
})
.then(function(body) {
return body.user;
});
It seems a bit more compliant with leading dots in https://github.com/airbnb/javascript#whitespace
Definitely leaning more to have .then on a new line per @getvega's last comment, and it does line up with the guides direction for long chains.
Looks a lot more readable.
I came here looking for indentation guidelines for promises and found this issue. I use the same indentation style as suggested by @getvega and agree with @JoshuaJones on it's readability. As for the original issue, as other have pointed out, it'd be a mix of the two options rather than just one or the other. The es6 fat arrows are nice for readability when you want to use closures, but there is also the benefit of the named functions for the stuff you want to reuse in other places. @PandaWhisperer makes good points too when catching rejections.
In general, I would like to promise practices to be as close to the use of async/await as possible. (currently stage 3)
Errors
@PandaWhisperer's error thing code is a good example, the first one avoiding Promise.reject and Promise.resolve is closer to what this would look like with async/await:
try {
const result = await getPromise();
if (!checkResult(result)) throw new Error("invalid result!");
doSomething(result);
} catch (errr) {
handleError(errr);
}
Anonymous vs named
Concerning @PandaWhisperer's original question: I think whether to use anonymous or named functions should really be up to the developer, based on his situation; As it is with synchronous code (should I extract this to be a subroutine or not?). Consider his exampled refactored into async/await:
try {
const result = await doSomething(foo);
const result2 = // ...
const result3 = // ...
// ...
} catch (errr) {
// ...
} finally {
// ...
}
Whether result2 is best obtained with a function doThis or with one or more lines of code really depends on the situation.
I'm not saying that because it depends on the situation with async/await, it also does for promises. I would actually prefer promises to be written with a set of well-named functions over them being written with a set of anonymous ones, but that's my personal preference; not what I would want in a style guide.
Line breaks
I like @getvega's suggestion to always put .then on a new line. This seems consistent with the chaining style.
Arrow functions
I especially like @getvega's arrow function suggestion. Applying this to his second example:
getPromise()
.then(response => response.body)
.then(body => body.user);
Makes it easy to see we could even turn this into:
getPromise().then(res => res.body.user);
Badabing!
Wow. This issue is more than a year old. Has the guide been updated with regards to promises? Just did a quick check and it seems that while it has been updated for ES6, promises still aren't mentioned anywhere.
FWIW these days I generally prefer the async/await style, but using generator functions and yield.
Duplicate of #597 as well.
We will be updating the guide to discuss Promises and async/await as soon as possible. Generators + yield would be an antipattern in whatever we write up, fwiw, as generators are synchronous.
Generators + yield would be an antipattern in whatever we write up, fwiw, as generators are synchronous.
How is that relevant? Yes, a generator runs synchronously, but only until it is eventually suspended when waiting for the promise to resolve. The promise still works asynchronously.
async/await are ES7 only, while generators and yield can be used today.
async/await is not in ES7, but may be in ES2017.
One of the primary issues is that generators can't be used without the regenerator runtime, which is very very heavyweight for dubious benefit. We don't yet endorse async/await because of the same requirement.
Okay, cool, I was not asking in a provoking way, more asking because it was not clear if Airbnb was using promises, because I thought this covered everything that Airbnb was using, also because this issue has been open for 2 years 😄
I was looking on some good advice for naming of promise functions. It seems that @ljharb closed the discussion "in favor of" this one (https://github.com/airbnb/javascript/issues/848). Sadly, I don't see any deep discussion on naming here either.
Even when only writing for myself I've found I've created all to many bugs without having a specific naming convention for my libraries that use promises. Yes, I realise documentation is the "real" way to do it, but because JS lacks typing and generally lacks hinting (I know there are solutions) it would be nice.
I agree that asyncMethod() is not really the right way to do things, as when you go into node a whole lot of std lib methods are "async by default" using callback method. Those that aren't end up being called syncMethod() so it just gets confusing.
That said, I don't have a great suggestion myself. I might try some of these and see how they look/feel in the code:
pMethod()
promMethod()
methodP()
I think the last one is likely the best because you're not starting with typing a "p" and the code completion would make more sense.
I'm sure others have way better ideas ..
I don't think a "p" notation is needed; functions returning booleans (predicates) aren't prefixed with "b" either.
Knowing what a function returns is something that ideally should be conveyed in the name - ie, a predicate named "isFoo" clearly returns a boolean - but the assumption should be that everything async returns a promise, and it should ideally be apparent in the name of the function that it's async. If it's not, documentation indeed is the proper way to do it (or more accurately, referring to the actual function definition).
@ljharb you make a great point, using booleans as an examples does help. The problem is (I only speak for myself) that "is" is specific to Booleans. "is" asks a question that begs a yes or no answer.
The same way that you'd expect "numberOfCats" to be an integer, or "amountPaid" to be a float.
The problem with assuming that "everything async returns a function" is that you're writing everything from scratch. Or even rewriting everything. A lot of the time you're not that lucky! I realise that means you're also going to not be lucky in the fact that libraries generally will lack some kind of formal naming. But isn't this discussion about trying to encourage people to best practices so you can at least hope?
When you look at code that use Promises, truthfully it ends up being obvious what is a Promise and what is not though, of course, as you'll always have then()s.
You should be wrapping any callback-taking async lib with a Promise-returning function.
I'd go with
return doSomething(foo).then(result => {
// ...
}).then(result2 =>{
// ...
}).then(result3 =>{
// ...
}).catch(error =>{
// ...
}).finally(() => {
// ...
})
Should be fine as long as you don't use this
@kevinsimper Indeed, this one of the oldest issues I've opened on Github that I still regularly get updates about.
At this point, I try to avoid any promise chaining whenever possible and just use async/await if possible, or the generator function-based workaround for pre-ES7 environments (usually with some syntactic sugar).
At least in CoffeeScript, this lets you write code that is practically identical with the new async/await syntax. In JavaScript, it's a fair bit uglier because you have to wrap your function definition into another function call.
Not sure what AirBnB's stance is on async/await (couldn't find it in the style guide, probably because it only deals with ES6), but looks like generators are not welcome. Interestingly, after 2 years, they still don't even mention promises in the style guide. 🤔
@PandaWhisperer we do not allow async/await, primarily because transpiling them requires regenerator-runtime, which is very heavyweight.
await, however, is mostly just sugar for nested promise .thens, and promise chaining is a perfectly fine pattern - one that you can't avoid knowing about or using, even with async functions, which are just sugar for a promise-returning function (and thus always require a .catch at least).
I just happened upon this conversation while browsing the internet for an answer to this exact question. However my personal convention, which I quite enjoy, involves replacing the get in method names with promise - so something like @tarwin's "numberOfCats" would become promiseNumberOfCats(): Promise<Number>, clearly signifying that the method promises to return the number of cats.
I've never been a fan of indenting chains however this especially applies to promises because their whole nature is to represent async code in linear style.
For me, first problem with indenting chains is that now closing brackets misalign, sometimes making you think you forgot a curly bracket.
Second, is that indentation sort of implies that indented methods are children of the first method, where for me they are more of siblings following each other in linear/sync fashion. That is what promises try to achieve in the first place.
So I usually go for this:
getPromise()
.then(function(response) {
return response.body;
})
.then(function(body) {
return body.user;
})
.catch(function(err) {
// handle error
});
or
getPromise()
.then(extract)
.then(parse)
.catch(handleError);
In my opinion this is more readable and looks more synchronous.
Code that's not sync shouldn't strive to look sync. That's not the "whole nature" of promises either - promises are composable.
Apart from verbosity, can anyone see anything wrong with using waitfor_ as a prefix for promise returning functions? To me it improves readability enormously (and thus reduces the possibility of coding errors), but maybe I am misleading myself in some cases by using such a prefix?
@surruk51 https://github.com/airbnb/javascript/issues/216#issuecomment-289577743, but also "wait for" is the wrong term imo, because "wait" connotes blocking.
As a bald statement I think you would be right. However the promise is in this context: .then(waitfor_something) .then(waitfor_somethingelse) i.e. it is not the machine waiting, but my logic.