node:test APIs returning Promises clashes with no-floating-promises lint rule

[JoshuaKGoldberg]

Version

21.5.0

Platform

n/a

Subsystem

node:test

What steps will reproduce the bug?

1. Write code that calls a describe, it, or test function imported from node:test
2. Lint that code with @typescript-eslint/no-floating-promises enabled

I put a standalone repro on https://github.com/JoshuaKGoldberg/repros/tree/node-test-no-floating-promises. From its README.md:

import { it } from "node:test";

it("", () => {});
// Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the `void` operator.
// eslint(@typescript-eslint/no-floating-promises)

How often does it reproduce? Is there a required condition?

100% of the time. The most common workarounds are to either disable the rule in test files or use the rule's ignoreVoid option.

What is the expected behavior? Why is that the expected behavior?

@typescript-eslint/no-floating-promises is enabled in the plugin:@typescript-eslint/recommended-type-checked config that is recommended to users as part of typed linting. I was expecting that the built-in functions for Node.js wouldn't directly trigger complaints in the built-in presets for typescript-eslint.

What do you see instead?

Lint failures out of the box.

---

As for what should be done: I'm not sure 🤔. If it were just up to me, I'd say switching the functions to have void returns ... or failing that, having their @types/node types changed to return void to indicate the returned values shouldn't be used.

How intentional is it that the functions' returned values are made available to users? I can see an argument that returning a Promise that rejects or resolves is a potentially useful feature for the functions...

Additional information

I wasn't sure whether to file this as an issue or discussion. It feels like an issue to me because common practice for many TypeScript projects has been to enable this rule, or previously TSLint's no-floating-promises. But I'm not a Node.js expert - apologies if I'm off base 🙂. If the rule is wrong to complain for some general-to-JavaScript reason, then I'd be happy to take lead on a general bug in typescript-eslint.

We first received this as a bug report in typescript-eslint/typescript-eslint#5231. We wontfixed that as it's not a good idea for a linter to add framework-specific behaviors. We're instead discussing adding an allow option to @typescript-eslint/no-floating-promises. But, that wouldn't be ideal for users IMO because then everyone would have to add an explicit allow to their ESLint configs or use a shared config/preset.

[aduh95]

/cc @nodejs/test_runner

[ljharb]

I've never seen a test framework where it returns anything but void/undefined; this feels like an oversight.

That said, an additional oversight would be that the promise currently returned from it needs to actually be handled by the test runner.

[bradzacher]

I've personally found it's kinda confusing because there's the top level promises which you shouldn't await and then there's the inner promises you should await. But there's the global functions and the inner functions passes into your test closure - all of the same names.

Also sometimes I wanted to await the promise within a test closure - sometimes I didn't.

I had to try combinations of things to figure out exactly what to do and why certain things didn't work.

[felixfbecker]

Is there anything speaking against just recommending (in examples in the docs) to await these promises, given we have top-level await? Since the test suite functions can be async (which is a useful feature because it means you can read fixtures from files etc) it seems logical to me that test()/it() should be awaited

[mcollina]

I think the no-floating-promises rule is problematic and sincerely limit the capability of frameworks.

The problem stem from the fact that promises could either be a request/response primitive, or a fire-and-forget one. The rule does not allow the maintainer/author of the library to indicate the distinction: there are certain promises that must be awaited, and others that can be completely avoided.

The no-floating—promises rule also apply to Thenables (which are not promise), which caused me various headaches in the past.

You can read the full explanation of all of this at typescript-eslint/typescript-eslint#2640.
What is missing in the rule is to allow maintainers to comment functions and let them bypass the rulez

Given the response in the linked issue, it does not seem there is any kind of will to have this sorted. Therefore, my recommendation is to stop using this rule, it prevents frameworks from using idiomatic and better DX.

Last but not least, I think the ship has sailed from changing this, because:

1. it solves a user need elegantly
2. changing it would be a semver-major change

I’m happy to talk to anybody on the tslint team about this.

[JoshuaKGoldberg]

Thanks for the detailed response @mcollina!

it solves a user need elegantly

Clarifying: what is that user need, and how often is it come up?

changing it would be a semver-major change

Clarifying: is that a blocker?

happy to talk

I would love to talk to you about this 😄. Will reach out to see what works best.

recommendation is to stop using this rule

Since you're posting a recommendation I feel obliged to explain why this rule has stayed so rigid despite its wide use 🙂:

there are certain promises that must be awaited, and others that can be completely avoided.
allow maintainers to comment functions and let them bypass the rule

That's a problem that we've continuously bumped against in trying to make the rule palatable. The vast, vast majority of the time (>99%), Promises cannot be completely avoided - regardless of what the framework author says. If the framework has a bug then crashes in the "floating" Promise will be ignored by the calling code. It's almost always an indication of non-idiomatic code to see floating Promises.

Not saying your counterpoints aren't valid, but: there's a reason why this rule is so often used and recommended. So it'd be good for us to figure out if there's a good path forward!

[bradzacher]

there are certain promises that must be awaited, and others that can be completely avoided.

To be clear: the rule does not prevent usages of fire-and-forget promises.
By default it promotes a style of explicitly annotating these kinds of promises (void promise;). Such a style allows a developer to signal their intent - "I am not awaiting this promise on purpose" - unlike the alternative (promise;) where the intent is ambiguous to future readers.

The rule does not allow the maintainer/author of the library to indicate the distinction

My question as a user of any framework would be "if I'm not supposed to await this promise - why did you return one to me?".

If the response is "you need to await it sometimes, RTFM" - things are no longer statically clear and we've added nuance to the API usage. Nuance is not necessarily an issue!

Instead the issue is that neither JS nor TS has a way to describe a "fire and forget" promise - so it's really hard to create generic static analysis for this case. This is doubly true when frameworks don't have hard-and-fast rules for what is fire-and-forget and what is not.

---

You're presenting this problem very "black and white" however in reality the problem is quite "grey".

As Josh mentioned - from our user's experiences and from our own experiences:

• the vast majority of promises should be awaited
• a small percentage that should be not awaited, but should be .catched
• a tiny fraction of promises can be "forgotten" (not awaited or caught).

From my personal experience it's easy to drop a lot of cases into that last bucket accidentally (by just forgetting an await/catch) or on purpose (by misunderstanding the usecase).
For example I upgraded my company's codebase from node 14 to node 16+ - which turned on the (great) flag --unhandled-rejections=strict but meant I had to cleanup hundreds of such bad cases.

The lint rule is great at flagging the first two cases and has a mechanism to explicitly annotate the 3rd.

From a user's (and framework author's) perspective the missing piece is "automated" skipping of the 3rd case as opposed to manual. Personally I prefer the explicit opt-out, but I do understand why people would like an implicit one.

[bradzacher]

All that being said - we're open to an option.

Our response to feature requests in our repo is often terse - as you would understand as a maintainer of node responding deeply to issues takes time and as volunteers it's not always possible for us to dump all the necessary context when rejecting a request.

We're open but we're also very cautious about it. An option to ignore specific cases opens up a footgun in the rule - it opens up a big vector in a user's codebase.

If a framework has a truly "fire and forget" promise then there's no bug vector at all and allowing configuration to ignore it is good. Though one could argue (as mentioned above) why the framework even returns a promise if its never meant to be awaited - that is separate to the rule.

If the framework has a promise that is "sometimes F&F" and "sometimes await/catch" then always ignoring it is bad and is a footgun. False negatives are an insideous thing as they are invisible. We always prefer to create a small percentage of visible false positives rather than the same number of invisible false negatives.
FPs can be seen and acknowledged - even if they are annoying. FNs cannot be known until they cause a bug.
FNs also harm user trust in linting - a user thinks "It was not caught by the rule and caused a bug - what other cases were missed?". This has a negative impact on the entire ecosystem!

With a nuanced rule like no-floating-promise - I hope you can see why a false positive is generally better than a false negative.

---

Again all that being said - we're open to it, but we definitely want to be sure it's done for the right reasons and in a way that is less likely to cause false negatives.

[bradzacher]

I’m happy to talk to anybody on the tslint team about this

Nitpick. TSLint is dead and has been for over 5 years.
We are the typescript-eslint team -- and we are happy to talk to you at length 😄.

For reference - calling us the tslint team is like if we were to call you guys the io.js team - in a round-about way it's "correct", but it's far from the preferred name.

[mcollina]

For reference - calling us the tslint team is like if we were to call you guys the io.js team - in a round-about way it's "correct", but it's far from the preferred name.

I'm sorry about this, I didn't keep track!
Fun fact: the Node.js TSC email is tsc@iojs.org, so calling us the io.js team might not be incorrect either!

---

From my personal experience it's easy to drop a lot of cases into that last bucket accidentally (by just forgetting an await/catch) or on purpose (by misunderstanding the usecase).
For example I upgraded my company's codebase from node 14 to node 16+ - which turned on the (great) flag --unhandled-rejections=strict but meant I had to cleanup hundreds of such bad cases.

The lint rule is great at flagging the first two cases and has a mechanism to explicitly annotate the 3rd.

What's the mechanism? Can we fix it in DefinitelyTyped (I'm a reviewer there) so everybody has a good experience? What's the point of changing the API?

@JoshuaKGoldberg pointed me to typescript-eslint/typescript-eslint#7008. I can't wait for this to land, so at least we will have a way to remove friction from developers. I hope this is also enabled by default by adding some kind of additional symbol/property so that frameworks can opt-in and solve this problem for their users.

---

I want to stress that the rule is black-and-white and put a drain on maintainers. I've been asked these questions a gazillion times and replied: "no, it's ok not to await those promises; we are handling it within the Framework."

Why such a convoluted APIs are needed? Because many Node.js APIs are not promise-friendly (starting from CommonJS). Specifically, there are complex issues when using promises with EventEmitter/Node.js Streams. Complex APIs with optional promises are needed to keep a nice DX for everybody, minus those who enable this rule (or use the default settings of typescript-eslint). For them, there is a lot of void to add everywhere, especially when using thenables/fluent APIs.

What's needed is an escape-hatch to allow maintainers to tell "it's ok to not await this promise/thenable", and turn that on by default.

---

The only alternative API design I could see for this issue is the following:

import { test, describe } from "node:test";

describe('something', async () => {
  await (test('aaa', async () => {
    // ...
  }).done)
})

I consider this a worse DX.

The alternative (which I pursued elsewhere) is not type the function as returning a Promise in the types. I think that's an option too, but it's a worse solution than having a nice escape hatch.