Bug: [no-floating-promises] False positive for spread arguments in then() #8958
Comments
|
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, 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 typesIn the playground link, you've done things a bit differently, so that 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 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 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 youAt 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);BugsThe 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! |
|
I would tend to agree. 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... |
|
@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. |
Before You File a Bug Report Please Confirm You Have Done The Following...
Playground Link
https://typescript-eslint.io/play#ts=5.4.3&fileType=.ts&code=C4TwDgpgBAShDOB7ANgN2gXigClQQ2QFcIAuKVRASwBMoAfKABQCdEBbS%2BCAGUoGsIAHgo0AfAEooGUeSrUA3AChQkWBABWEAMbApOZhDxIAdgH4yhY32OIA7scnTZNJYq2Jj8XQG8oBpGgQADR%2BGtq6AL5k3v4o6GRwAeghBpo6CWE6EXos7JwQAHS2lMAAFolxEMzwwnKi2OJK7p66sYHVZADaFYEhcGnAALp6nW3JoQODrgD001AAYgRcUGCI8CWU6FC2pRDGoUlV8FB4BlDwYAZ4tJTGwIhQZXsNirkcXAVjEA0FT8bYBUBX2qjUUilmUAAchAAOZ4YCbaCvVjvQpfH5-bDA%2BCdAAMgxSCEq1U6AEZBqCgA&eslintrc=N4KABGBEBOCuA2BTAzpAXGYkACAXAngA4oDG0AlobgLQrzkB2uA9AwPbUBm8bAhrowDm1QtDYBbcshTooiaGOiQAviGVA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA&tokens=false
Repro Code
ESLint Config
tsconfig
{ "compilerOptions": { // ... } }Expected Result
I expect no errors on line 4 when spreading resolvers into
then().Actual Result
There's an error on line 4:
Additional Info
No response
The text was updated successfully, but these errors were encountered: