Bug: [no-floating-promises] False positive for spread arguments in then() about typescript-eslint

[bradzacher]

I would tend to agree.
I think this is something that's okay sitting unhandled given this complexity and how rare it is - most people will not create a new array to house two parameters to spread it and would instead just pass the two parameters directly.

I struggle to think of real world code where you would actively chose to write code like this, TBH. The array form seems very, very opaque as an API design choice and only really supports void-returning promises. It also doesn't let you handle errors...

[jamonserrano]

@kirkwaiblinger Thanks for the thorough response!

In our application we use events to bridge the gap between React and the communication layer, and this solution intended to improve the developer experience of handling requests. The resolvers were not extracted from actual promises, they were just there to provide a familiar API. In the end I just delegated these events to a wrapper function that handles the promise, so the resolvers are now completely hidden from plain sight.

I agree it's probably an edge-case; I expected it to work with tuples though, but your explanation makes complete sense, thank you.

[kirkwaiblinger]

Thanks for the report @jamonserrano!

This is an interesting one. I definitely see what you're saying, but I'm uncertain there's a great solution.

In particular, with the exmple you've given, resolvers will have type Array<(value: void | PromiseLike<void>) => void>. You and I know that that array has 2 elements, because we can see where it's been constructed, but we have no way to know this in general.

const resolvers: Array<(value: void | PromiseLike<void>) => void> = [];

const { resolve, reject } = Promise.withResolvers<void>();
if (condition) {
   resolvers.push(resolve, reject);
} else if (anotherCondition) {
   resolvers.push(resolve);
} else {
   // do nothing
}

// resolvers might have 0, 1, or 2 elements.
Promise.reject().then(...resolvers);

Tuple types

In the playground link, you've done things a bit differently, so that resolvers is a tuple rather than an array. Tuples do have fixed length, so I agree with you that we could handle that case.

declare const resolvers: [() => void, () => void];
// we have enough information here with the types to be able to say that a catch callback is provided.
Promise.reject().then(...resolvers);

We have done something like this before use-unknown-in-catch-callback-variables, though it's never going to handle all cases perfectly, since you can have unions of tuple and things like that. To be honest, it's a fun programming puzzle, but maybe of questionable value 😆.

Please let us know if you have a natural use case where this would be handy, though! My thought is that you kind of have to go out of your way to create a tuple in the first place, either by as const or explicitly const resolvers: [Resolve, Reject] = ..., so I'm unclear how this might occur organically.

Even if we tightened up the checks around tuples, this wouldn't address the code you've provided in the issue report.

What I would do if I were you

At first glance, I would probably recommend anyways refactoring your code to pass the 2 explicit arguments. That makes it easy for a human to read and for our rule to inspect correctly 🙂. Do you have a concrete use case where that is unwieldy?

const [resolve, reject] = resolvers;
Promise.reject().then(resolve, reject);

Bugs

The rule does have at least one related bug, though, in that it doesn't currently distinguish between normal and rest args at all. So you can trick it with

const emptyResolvers: Array<() => void> = [];
// error, correct
Promise.reject().then(...emptyResolvers);
// no error, bug! The rule is tricked by there being two arguments,
Promise.reject().then(...emptyResolvers, ...emptyResolvers);

That much we should definitely fix.

---

Would love to hear your thoughts on the above if you have time to read through it!