[await-thenable] warn against passing non-promise values to promise aggregators (Promise.all, Promise.allSettled, Promise.race)
#1804
Comments
|
I am unable to reproduce this against master
Could you please provide a repro repo for this to help me investigate? |
|
@bradzacher, Thank you for your quick reply, I will create the repro repo, In the meantime, from the image you shared I can see that you added this test case in the valid property, the issue is that the code should be invalid and throw an error. |
|
wait sorry, I misunderstood your OP. Why should it be invalid? |
You are right, While it is valid JavaScript to pass non-promise elements to const promise1 = Promise.resolve();
const promise2 = Promise.resolve();
const test1 = (): void => {};
export const run = async (): Promise<void> => {
await Promise.all([promise1, promise2, test1()]);
};Here it makes no sense at all to use |
|
AHHHHHH okay i'm with you now. Sorry, slow morning. You're saying that you want the rule to warn against passing non-promise values into the native promise aggregation functions ( Even though it is type-safe and runtime-safe, it may not make logical sense to do so. If yes, this sounds perfectly reasonable. Would be good as an option on the rule. |
|
@bradzacher Precisely 😄 |
await of a non-Promise does not work if wrapped with Promise.allPromise.all, Promise.allSettled, Promise.race)
|
Refactored some code recently where I saw various calls like: const someFn = () => {...};
await Promise.all([await someFn(), await someOtherFn()])It seems this should also prevent that type of problem? Or is there an alternative? |
|
No, there are no rules to prevent this code. Please file a new issue, and happy to accept a PR |
|
I understand that, I meant that fixing this issue would additionally fix the use case I mentioned. |
|
Sorry, I guess that potentially it would be if the types work out (i.e. your functions don't return something weird like Though it wouldn't be warning against the fact that you have |
|
I think that if functions return I think the cryptic error would be ok, at least the user will get a clue that something is wrong with the code. Perhaps it could even provide a hint like |
|
Days since last bug caused by this: 0 |
Repro
{ "rules": { "@typescript-eslint/await-thenable": "error" } }Expected Result
The rule should show a linting error:
Actual Result
No liniting error.
Additional Info
Versions
@typescript-eslint/eslint-plugin2.24.0@typescript-eslint/parser2.24.0TypeScript3.8.3ESLint6.8.0node12.10.0npm6.10.3The text was updated successfully, but these errors were encountered: