[return-await] Recommendations on return-await #1378
Comments
mightyiam
added
package: eslint-plugin
bradzacher
added
question
|
The bulk of the arguments in standard/standard#1442 do not apply, because we are using typescript. Type checking very easily catches all of these "refactoring traps". Better still, if you use There is a lot of debate around as to whether or not you should await a returned promise. There are probably some (negligible) perf benefits to not using It's ultimately down to personal preference. What do you like to do? Do you like the stack trace? Do you prefer the style of not having the extra await? Etc PERSONALLY: I think that it's better to not await a returned promise, because I don't think it's right to have the useless await. But I think you should always await the promise if the return is inside a try/catch, so that the error handling actually applies. My personal recommendation would be to use our |
@bradzacher could you confirm for me if you meant |
|
Good catch, sorry. Meant |
|
Thank you, @bradzacher. Keep up the excellent work. |
We actually want a rule that enforces to *always use return await*.
Reasoning: Putting try/catch around a return without await is a footgun.
try {
return somethingAsync();
} catch (error) { <-- will never be caught
}
Further discussions:
- eslint/eslint#12246
- mightyiam/eslint-config-love#206
- typescript-eslint/typescript-eslint#1378
|
Sorry to drag up an old thread 🙈 @bradzacher would you mind quickly elaborating on why you think the extra stack frame is a "red herring"? One thing that I've had a few times are variations of the following problem: I get an Had the code-base been using My experience is that the stack traces provides much more usable information when having It should be noted that we've only recently started embracing The parts that we have converted to async await has much better stack traces, and I really like that the extra frames are in. Sorry, this got a bit longer than I intended 😄, would love to hear your thoughts, I'm always open to seeing the other side! |
@bradzacher Why is it a red herring? Example: // Assume that User.findByOrFail is a database method that queries by a field on the
// users table and throws a ResourceNotFoundError if it can't be found, otherwise
// returns the first match.
async function getUser (request) {
if (request.loginMethod === 'jwt') {
if (!request.jwt || !request.jwt.valid) throw new Error('Invalid JWT')
return await User.findByOrFail('id', request.jwt.userId)
} else if (request.loginMethod === 'apiKey') {
return await User.findByOrFail('apiKey', request.apiKey)
} else if (request.loginMethod === 'serviceToken' && request.isTrustedSource) {
// COPY-PASTE ERROR HERE: should be request.serviceToken and not request.apiKey
return await User.findByOrFail('serviceToken', request.apiKey)
} else {
throw new Error('Unknown login method')
}
}My example would throw a Without the Not very helpful. Yes I know I'm missing type definitions here, but I can't see how they would fix it. This example is from a JS case but I think it should apply here too. |
|
Is this a missing stack trace frames issue before it is a linting issue? Should the stack trace contain the missing frame regardless of |
I don't think that a conforming engine could add the frame unless there is an |
|
Hm, interestingly in Chrome devtools it is there. I guess it keeps a reference of the stack at the time of function invocation, which is actually a good idea I believe. After all, the actual invocation is inside the other function, regardless of whether it waits for completion or not. |
|
In an example like yours, where there are multiple calls to exactly the same async function, then sure there can be some use to that extra frame. Though chances are you should be able to tell based on the next frame which line was called. I don't know your codebase, but it's unlikely you pass the arguments down 5-10 layers of function calls, all with no awaits, and no logs, and then await the promise at the top level. Chances are you've got code that looks something like: async function handleRequest(requestFromApi) {
log(request);
// ...
await getUser(request);
// ...
}And in that case, it's not entirely important that you have that extra stack frame, because the important part is the or in most code cases, something more like: async function doSomethingA(apiKey) {
// ...
await getUser('apiKey', apiKey);
// ...
}
async function doSomethingB(serviceToken) {
// ...
await getUser('serviceToken', serviceToken);
// ...
} In this case you don't care about that stack frame, because it's trivial to figure it out, and it probably doesn't help to know it. The other problem you've got is that the extra await can seriously impact performance due to the suspension introduced by the await. async function returnAwait() { return await Promise.resolve() }
async function noReturnAwait() { return Promise.resolve() }
async function main() {
console.time('returnAwait')
for (let i = 0; i < 100000; i+=1) {
await returnAwait();
}
console.timeEnd('returnAwait');
console.time('noReturnAwait')
for (let i = 0; i < 100000; i+=1) {
await noReturnAwait();
}
console.timeEnd('noReturnAwait');
}
main();
You can imagine if you have multiple async function returnAwait(arg) {
if (arg === 0) {
return await Promise.resolve()
}
arg -= 1;
return await returnAwait(arg);
}
async function noReturnAwait(arg) {
if (arg === 0) {
return await Promise.resolve()
}
arg -= 1;
return noReturnAwait(arg);
}
async function main() {
console.time('returnAwait')
for (let i = 0; i < 100000; i+=1) {
await returnAwait(5);
}
console.timeEnd('returnAwait');
console.time('noReturnAwait')
for (let i = 0; i < 100000; i+=1) {
await noReturnAwait(5);
}
console.timeEnd('noReturnAwait');
}
main();
Ultimately, I guess the question is, "how much is that extra stack frame worth to you?" Would you rather take the perf impact of a There's no "right" answer here, I don't think there's a true "hard and fast rule" which should say "always always do this". My recommendation is still to use our |
|
That's interesting, I did my own test before and the result was that it does not cause another tick delay. (In actual processing time it added 12 microseconds per iteration in my tests.) Also, I see that when returning 123 instead of undefined in the resolved promise, the time for the return await doesn't change while the time for the non-awaiting return increases. |
I don't see that same impact. I get the same average runtimes regardless of what the promise resolves to.
It does not, but it does cause a microtask to be queued. https://tc39.es/ecma262/#await
https://tc39.es/ecma262/#sec-performpromisethen
And the spec does not explicitly declare when and how the queues are to be processed. https://tc39.es/ecma262/#sec-jobs-and-job-queues
It only dictates that
There are potentially separate queues for promises and timeouts, so in your examples, it's perfectly reasonable for the engine to enqueue and dequeue the promise result immediately, before dequeueing the The thing I'm getting at is that promises and how they are handled is sometimes straightforward, and sometimes not. With all the enqueueing happening, it's reasonable to assume that your application might enqueue a long running promise resolution between the OTOH, if you don't use Also every It's a tradeoff. Potential better devx for potential impact to the end user. But the more research I do into this, the firmer I become in my recommendation of using |
I don't think that calling it a "serious impact" is fair, in an actual event looped single threaded application yielding to the runtime more often could actually increase throughput. Even so, your benchmark only shows a few micro-seconds of overhead of calling e.g. benchmarking I'd definitely take that super-small (micro-seconds) overhead to aid in debugability.
Even though though you sometimes, maybe even in many cases, can figure out what the missing frames are, I don't see what negative it would do to just have them there from the beginning.
To me, this sounds very lot like premature optimisation. I cannot possible see an app where extra |
If your application does make copious use of rounding, then I would recommend it. Though I would recommend instead just using a babel transform to optimise it away to keep things clean and clear for developers. It's important to evaluate your codebase to look for common patterns, and figure out if it's good or bad.
Sure, but IMO this straw is unnecessary weight, and shouldn't be introduced in the first place This is something we clearly aren't going to agree upon, and that's okay. Linting is configurable because it is all subjective, and opinionated. You're free to use it in your codebase if you think it improves your devx. My recommendation will remain as mentioned. |
Sound good 👍 |
Thank you for this super helpful project!
I am the main maintainer of https://github.com/standard/eslint-config-standard-with-typescript but I'm not a computer science person and sometimes struggle understanding linting rules.
One such example where I struggle is with the return-await rule.
eslint-config-standard-with-typescript aims to provide a stricter set of rules than plain standard using this project's parser and plugin.
Is there an obvious decision regarding return-await — whether it is generally recommended in any form, please? I do see that it is not included in the exported recommended config. I would appreciate all the input on this.
For reference, we have a discussion on no-return-await and ESLint had one as well.
The text was updated successfully, but these errors were encountered: