ESLint: Require await inside async functions #5263
Conversation
Overall package sizeSelf size: 8.75 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.4.0 | 29.44 MB | 29.44 MB | | @datadog/native-appsec | 8.4.0 | 19.25 MB | 19.26 MB | | @datadog/native-iast-taint-tracking | 3.3.0 | 13.77 MB | 13.78 MB | | @datadog/pprof | 5.5.1 | 9.79 MB | 10.17 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.8.0 | 2.6 MB | 2.74 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 3.1.0 | 1.06 MB | 1.46 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.11.2 | 112.74 kB | 835.4 kB | | source-map | 0.7.4 | 226 kB | 226 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.2 | 53.63 kB | 53.63 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.3.1 | 25.21 kB | 25.21 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | semifies | 1.0.0 | 15.84 kB | 15.84 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | ttl-set | 1.0.0 | 4.61 kB | 9.69 kB | | path-to-regexp | 0.1.12 | 6.6 kB | 6.6 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5263 +/- ##
==========================================
+ Coverage 80.80% 80.91% +0.11%
==========================================
Files 487 488 +1
Lines 21710 21817 +107
==========================================
+ Hits 17542 17654 +112
+ Misses 4168 4163 -5 ☔ View full report in Codecov by Sentry. |
Datadog ReportBranch report: ✅ 0 Failed, 663 Passed, 0 Skipped, 10m 33.82s Total Time |
BenchmarksBenchmark execution time: 2025-02-19 06:34:00 Comparing candidate commit 8a0931a in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 918 metrics, 15 unstable metrics. |
|
Actually, can we instead have a rule to block async/await entirely? Promises too. We've had a non-written rule about never using promises in the codebase since day 1 because it can slow down |
|
@rochdev I wouldn't mind doing that, but I feel that's a different PR |
There was a problem hiding this comment.
LGTM except that one request change, and I'd like to make sure @juan-fernandez reviews the test-framework instrumentation code.
watson
force-pushed
the
watson/eslint-require-await
branch
from
d78565a to
25835a5
Compare
If a function is marked as async, V8 will create a few extra promises behind the scenes when awaiting it. If the function doesn't itself contain any awaits, these extra promises just adds overhead that isn't needed. The `require-await` rule isn't enabled in tests, where the extra created promises doesn't harm anyone.
watson
force-pushed
the
watson/eslint-require-await
branch
from
25835a5 to
8a0931a
Compare
| @@ -10,6 +10,7 @@ addHook({ name: 'apollo-server-core', file: 'dist/runHttpQuery.js', versions: [' | |||
| const HttpQueryError = runHttpQueryModule.HttpQueryError | |||
|
|
|||
| shimmer.wrap(runHttpQueryModule, 'runHttpQuery', function wrapRunHttpQuery (originalRunHttpQuery) { | |||
| // The original function is async, but no need to mark it as such as long as it returns a promise | |||
There was a problem hiding this comment.
Should we really add these comments? While reviewing it, I also thought about it and it seemed fine without, since the method should always return what ever it originally does.
Obviously we may not change that, but it should also never be the case, right?
I am just in favor of not having comments as they are easily outdated, forgotten and often do not help a lot.
There was a problem hiding this comment.
not for this case, but for the test optimization cases, it'll help test opt devs 😄 , as it helps highlighting that async operations are indeed allowed in this wrapper function. Removing it means you either have to remember or check the test framework's (or library in general) original code
There was a problem hiding this comment.
I was on the fence about this as well. The reason I ended up adding the comments in the end was because of the risk involved. If we ever decided to return something else than the return value of the original function, we needed to ensure that it was wrapped in a promise. If we didn't, something would break. So I went with the better-safe-than-sorry approach 🤷
I am just in favor of not having comments as they are easily outdated
Unless the original function is changed to not be async anymore, the comment is never outdated, right?
There was a problem hiding this comment.
I'd argue it's not necessary for the comment to elaborate on what did the function use to be before the change. "Returns a promise" feels enough to me. Or "Returns a promise, can be awaited."
If you want a bit more formalism, we can even put a JSDoc annotation on it?
/** @returns {Promise<…>} */
(I don't know what value type is the returned promise in question supposed to carry, hence ellipsis.)
There was a problem hiding this comment.
test optimization's perspective here.
I'm not against the change but I don't find it particularly useful: the asyncs need to be replaced by a comment or it'll make it harder to know that an async operation is possible in the wrapper. This makes legibility worse IMO.
As for the performance improvements: I understand the improvement, but is it really applying? These wrapper functions are async because the original ones are async. Example: https://github.com/jestjs/jest/blob/93ca9d3cf6db4b88393ab02b8d0007bcdf30e4bb/packages/jest-reporters/src/CoverageReporter.ts#L107
is wrapping an async function into a sync one what we want here?
This said, again, I don't oppose the change 😄
async/await is totally fine in tests |
Absolutely, by codebase I meant the source code, not the tests, and even that has exceptions today (for example I believe at least profiling has been using promises for a while) but the main reason it was blocked in most places is that historically it would cause issues with |
What does this PR do?
Enable the
require-awaitESLint rule. The rule isn't enabled in tests, where the extra created promises doesn't harm anyone.Motivation
If a function is marked as async, V8 will create a few extra promises behind the scenes when awaiting it. If the function doesn't itself contain any awaits, these extra promises just adds overhead that isn't needed.
Plugin Checklist
Additional Notes
Be aware: This might break some things that expect the function to return a promise. We'll have to go over each of the places where the PR removes the
asynckeyword and check if the function returns something that should be a promise, but isn't.There might be a few places, especially places were we use
shimmer.wrapwhere we might actually want to keep theasynckeyword!