-
Notifications
You must be signed in to change notification settings - Fork 26.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Best practices for promises? #216
Comments
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:
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:
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:
|
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, |
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 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 :
Instead of
|
About best practices, seems like you all put the What do you think of
It seems a bit more compliant with leading dots in https://github.com/airbnb/javascript#whitespace |
Definitely leaning more to have 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 Errors @PandaWhisperer's error thing code is a good example, the first one avoiding 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 try {
const result = await doSomething(foo);
const result2 = // ...
const result3 = // ...
// ...
} catch (errr) {
// ...
} finally {
// ...
} Whether I'm not saying that because it depends on the situation with Line breaks I like @getvega's suggestion to always put 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 |
Duplicate of #597 as well. We will be updating the guide to discuss Promises and |
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.
|
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 |
You aren't using Promises at Airbnb? |
Of course we do. |
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 (#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 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:
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 |
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 |
@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 At least in CoffeeScript, this lets you write code that is practically identical with the new Not sure what AirBnB's stance is on |
@PandaWhisperer we do not allow
|
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 |
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. |
@ljharb I agree with you. And chaining looks more 'functional'. |
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 #216 (comment), 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: |
there's no need for the prefix there tho; you can have the camelCase name of the function make it clear that it's making an async call - but also, its presence in the |
Chaining promises is fine but the chain should not go beyond 2 max. 3 |
Here is an eslint plugin for promises that the styleguide could probably use (with some modifications): https://github.com/xjamundx/eslint-plugin-promise |
@ljharb do you not use |
@antgonzales absolutely we do not, we use airbnb-browser-shims. |
Regarding naming, I have been using the unofficial Observable '$' convention in codebases where I do not have access to Observables. |
If you aren’t using a type system, such as TypeScript or Flow that shows the return type of the function, having a naming convention is beneficial. We have, before we introduced TypeScript, used |
It seems that most people here prefer the indented new line version of chained thens, like so whenSomethingHappens()
.then(whenSomethingElse)
.catch(console.log)
.finally( () => console.log("finally"))
; I personally prefer to not indent, because
whenSomethingHappens()
.then(whenSomethingElse)
.catch(console.log)
.finally( () => console.log("finally"));
// If using await
const something = await whenSomethingHappens();
try {
await whenSomethingElse()
} catch(e) {
console.log(e)
}
console.log("finally"); |
I like the inline definitions chained with each then/catch indented, but the leading periods bug me a little. I leave them at the end like my semicolons, e.g. return doSomething(foo).
then(result => {
// ...
}).
then(result2 =>{
// ...
}).
then(result3 =>{
// ...
}).
catch(error =>{
// ...
}).
finally(() => {
// ...
}); |
@popham fwiw, our guide strictly requires always using leading dots (as is conventional in the JS ecosystem) |
@ljharb, thanks for the feedback. I figured that I was cutting against the grain which is why I found this issue. I agree with the reasoning from 19.6, but.... I'm ignoring the Eslint suggestions, because I like chaining repetitions of the same call on a single line, e.g. foo.
bar().
baz() |
That the linter permits it isn't important; the guide prohibits it. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
To use a promise in a React component, you can use the Promise constructor to create a new promise and use the .then() method to specify a callback function to be executed when the promise is resolved. Here's an example of using a promise to fetch data from an API and then rendering a component with the fetched data: function Example() { useEffect(() => {
}, []); // Render the component {data ? ( {data.message} ) : ( Loading data... )} ); } |
Thank you! |
thanks for the details |
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
Option 2: named functions
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?
The text was updated successfully, but these errors were encountered: