-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Enhancement: [no-floating-promises] Option to only check within async functions #9061
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
I'm not quite sure I understand what you mean by this. Could you give an example? Have you considered the It adds a declarative way for a developer to declare "I am intentionally not awaiting this promise" and it works in both sync and async contexts. A lot of people like it because it is "symmetrical" with the Personally I don't think this is a good option because often times you can create a promise and forget to await it, even in a sync context. If you ignore all promises in sync contexts then it's very easy to create floating promises accidentally - as in - you meant for it to be an async function but you forgot to do. So. That is a massive bug vector in code! |
Unfortunately we have some developers that will see any tool we give as a hammer so I don't want to introduce (I guarantee if we turn on a no-empty-catch rule people will start just copy-pasting comments. I previously opened an issue because some devs write
async function foo() {
try {
await somePromise;
} catch (error) {
logger.log(error);
}
}
// needs .catch or void to satisfy linter even though this code should be safe
foo(); |
What happens when Jokes aside - #8404 is an enhancement that would allow you to build tooling for "guaranteed infallible promises" and have the rule understand that.
I'm not sure I understand - how would "allow floating promises in sync contexts" be any better than "use At meta the pattern we used was a function like so: function promiseDone(p: Promise<any>): void {
p.catch(someGlobalPromiseErrorLogger);
}
useEffect(() => {
promiseDone(Promise.resolve());
}); So like |
Thanks for showing the pattern from meta, that's good to know and something to investigate for us.
We've turned this on since :) It solved that workaround but now unfortunately developers just use empty catch.
I don't want to allow using |
It's not! Many codebases use it because it provides that nice, self-documenting, no-disable-comment way to mark a promise as intentionally floating. Josh calls it legacy because he doesn't like it and it originally became a part of the rule by accident. Way way back in the day someone did it by accident and asked for an option to catch it. And when it was added people learned it and liked it and so it became a common style. At Canva we use it and people like it because it means no disable comments and no empty catches. I think you're ultimately thinking about this too defensively.
You could replace void with literally any concept and have the same outcome! Disable comments, consuming functions, empty catches, no-call catches, etc. Even applies to any rule really as all rules enforce specific styles that people may copy and paste and misuse. People will need to learn your codebase style and convention - there's likely many patterns within your codebases that are considered idiomatic style internally, but that are not used externally. It's a very common phenomenon in codebases as they evolve naturally! Put it this way: From my experience working within large react codebases (fb and canva) - you're more likely to have people forget to make a function async than you are to have people misuse the "anti-floating" tool. And both result in the same problem - that there is a floating promise - but the former is harder to track down because you need to find an unannotsted, floating promise. Whereas the void approach is very clear - just look for the void! All that being said - I think ultimately what you want is #8404. You want to mark a promise as infallible and that is what that issue will allow you to do. Ignoring promises in sync contexts is just a workaround for the issue of the rule reporting on infallible promises. |
Sounds good, I'll explore using |
Before You File a Proposal Please Confirm You Have Done The Following...
My proposal is suitable for this project
Link to the rule's documentation
https://typescript-eslint.io/rules/no-floating-promises/
Description
no-floating-promises does two related things:
Because no-floating-promises has no good way to check if errors are caught in a function (ie using try-catch), the requirement to add
.catch
everywhere has encouraged developers to find workarounds like adding empty catch handlers that suppress other checks like the unhandled promise rejection event. We've decided to disable the rule in our codebase and push developers to look at unhandled promise rejections errors in telemetry instead.However, it would still be nice to keep a version of the rule that helps us remember to use
await
inside async functions. I'd like to see a version of the rule that only runs on promises inside async functions (and maybe runs on promises inside a.then
chain) and ignores promises called from synchronous functions likeReact.useEffect
.Fail
Pass
Additional Info
No response
The text was updated successfully, but these errors were encountered: