Description
What version of this package are you using?
14.3.1
What problem do you want to solve?
The no-return-await
rule introduced in #695 is the only one with which I continue to disagree, and I think since the arrival of async stack traces in Node, there is now another reason to rethink it.
Therefore I want to open a discussion here. My point is that the no-return-await
rule (which disallows "redundant" use of return await promise
) has two negative effects:
1) It reduces the usefulness of error stack traces
Node now has zero-cost async stack traces with the --async-stack-traces
flag. There, it makes a difference whether return promise
or return await promise
is used - the former omits a stack frame.
Example:
const delay = require('util').promisify(setTimeout)
const throws = async () => { await delay(100); throw new Error('oh noez') }
const a = async () => { return throws() }
const b = async () => { return await throws() }
async function main () {
try { await a() } catch (e) { console.error(e) }
try { await b() } catch (e) { console.error(e) }
}
main()
Result:
david@CHE-X1:~/z/Temp $ node --async-stack-traces example.js
Error: oh noez
at throws (/mnt/c/Users/david/Temp/example.js:2:54)
at async main (/mnt/c/Users/david/Temp/example.js:8:8)
Error: oh noez
at throws (/mnt/c/Users/david/Temp/example.js:2:54)
at async b (/mnt/c/Users/david/Temp/example.js:5:32)
at async main (/mnt/c/Users/david/Temp/example.js:9:8)
Note how there is no stack frame for a
.
2) It lays out a trap for code refactoring by making it easy to overlook that an async function is being called
Example with subtle unintended behavior:
return somethingAsync()
// refactor to
try {
return somethingAsync()
} catch (e) {
// ...
}
// whoops, now suddenly it makes a difference - errors are caught only
// if they happen in the synchronous part of `somethingAsync` - hard to spot!
Example with more obvious unintended behavior:
return somethingAsync()
// refactor to
return somethingAsync() || null
// whoops, that did nothing, because the promise won't be null
Another example:
return somethingAsync()
// refactor to
const value = somethingAsync()
await moreStuff()
return value
// whoops now somethingAsync runs in parallel to moreStuff, which
// may not be immediately obvious and cause a race condition
Or even:
return somethingAsync()
// refactor to
return { value: somethingAsync() }
// whoops now the outer code gets a promise in an unexpected place
// and if it previously was a boolean, it may not even be noticed, but
// conditions will now get always a truthy value...!
There are many more examples like this which don't immediately crash (it is easy to spot something like somethingAsync().toUpperCase() is not a function
, but my examples above are more subtle). If I modify existing code, I may not be familiar with every function that is being called (and I may even use a refactor tool where I just select the returned value and extract to a variable for example). Usually, I'd easily recognize that await somethingAsync()
should probably stay this way, but with return somethingAsync()
I may not realize that in any place other than the return
statement, I'd need an await
in front (and even if it stays in a return
, other things like the aforementioned try
block may suddenly require a change here - and imagine that the return
is somewhere inside a larger block of code which I'm now wrapping with try
)!
I had all of these issues from my examples happen in real life.
To work around this issue, I manually mark return somethingAsync()
with a comment, e.g. return somethingAsync() // async
but that seems ugly and much less useful than having the obvious await
there (plus, it doesn't solve the try
case).
What do you think?
What do you think is the correct solution to this problem?
Remove the no-return-await
rule
Are you willing to submit a pull request to implement this change?
I guess so...? (but I guess it would be easier for you)
Metadata
Assignees
Labels
Type
Projects
Status
Done