Skip to content

[no-floating-promises] Provide a configuration option or other method to mark a Promise-returning function as "safe" #4722

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

Closed
3 tasks done
ArcanoxDragon opened this issue Mar 23, 2022 · 4 comments
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin wontfix This will not be worked on

Comments

@ArcanoxDragon
Copy link

ArcanoxDragon commented Mar 23, 2022

Repro

I have encountered some cases while cleaning up some old JS code for my company (and converting it to TS in the process) where I want to register a .then() action on a Promise that I know will never be rejected. I don't want to have to use void whatever().then(...) on every single instance where I am dealing with one of these promises, as they may be used very frequently and having to remember to put void before every single usage of a non-rejecting promise is tedious (and ugly). I still want the warning to appear for promises that could be rejected, however, in case somebody forgets to handle an actual situation where a rejection might occur.

One specific example is in an AJAX helper method I created that will normalize any AJAX request (be it from one of our APIs or from an MVC backend), show an error message if it failed for some reason, or resolve a promise if it was successful. Here is some simplified pseudocode that should convey the general idea:

function sendAjaxAsync<Result = string>(options): Promise<Result> {
	return new Promise<Result>(res => {
		makeRequestAndNormalizeResponse(options).then(response => {
			if (response.isSuccessful) {
				res(response.result);
			} else {
				showErrorMessage(response.errorMessage);
			}
		});
	});
}

let myButton = $("#myButton");
let myContainer = $("#myContainer");

myButton.on("click", () => sendAjaxAsync({ url: "/foo/bar/baz" }).then(html => myContainer.append(html)));

If the request succeeds, the HTML from the response will be appended to myContainer. If it fails for some reason, sendAjaxAsync will show an appropriate error message to the user, and the continuation will not run. There will be no runtime errors or rejections at all.

Another use case might be something like a wrapper around setTimeout where the returned Promise is absolutely guaranteed to never reject ever, and it wouldn't even make sense to consider the possibility of a rejection:

// never rejects
function doSomething(): Promise<void> {
	/* ... */
}

// never rejects
function waitFor(ms: number): Promise<void> {
	return new Promise(res => setTimeout(res, ms));
}

let myButton = $("#myButton");
let myMessage = $("#myMessage");

myButton.on("click", () => {
	doSomething()
		.then(() => myMessage.show())
		.then(() => waitFor(2000))
		.then(() => myMessage.hide());
});

I'm sure there are plenty of other similar cases out there as well.

I am proposing a configuration rule such as the following to tell the no-floating-promises rule that the Promise returned by certain functions will NEVER be rejected so that a simple single-callback .then() is enough for it to be treated as "not floating":

{
    "rules": {
        "@typescript-eslint/no-floating-promises": ["warn", {
            "ignoredFunctions": [
                "doSomething",
                "waitFor"
            ]
        }]
    }
}

There may be a more elegant and/or flexible way of specifying this option; the example above is just one possibility.

Expected Result

I am expecting that, with the above hypothetical configuration, the example code would not have any linter warnings whatsoever.

Actual Result

Because no-floating-promises currently assumes that every Promise might be rejected, and thus must have a rejection handler registered 100% of the time, the above example raises a linter warning:

image

Additional Info

Versions

package version
@typescript-eslint/eslint-plugin 5.16.0
@typescript-eslint/parser 5.16.0
TypeScript 4.6.2
ESLint 8.10.0
node 16.12.0
@ArcanoxDragon ArcanoxDragon added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Mar 23, 2022
@bradzacher
Copy link
Member

Adding void is the recommended way to signal that you have intentionally left a promise floating. It's a good practice because, though it's verbose, it clearly signals you intentionally don't care about catching. Which self documents your code.

Otherwise using await would also do this. The rule doesn't expect await to be surrounded in a catch. No need to explicitly catch the await because your function never throws.


This isn't really possible to do because once the function returns - all promises are equal. The only way we could possibly do this is specifically for chained calls. We couldn't track through variable assignments.

Considering that the vast majority of codebases are using async/await, the applicability of an option that only ignores floating promises if there is a chained then and no catch would be very, very narrow.

As such I don't think this is a good inclusion in the rule.

Thanks for raising an issue!

@bradzacher bradzacher added wontfix This will not be worked on enhancement: plugin rule option New rule option for an existing eslint-plugin rule and removed triage Waiting for team members to take a look labels Mar 25, 2022
@vire
Copy link

vire commented May 19, 2022

Adding void is the recommended way to signal that you have intentionally left a promise floating. It's a good practice because, though it's verbose, it clearly signals you intentionally don't care about catching. Which self documents your code.

I don't agree here, if developers start adding void just to silence the ESLint rule, it becomes impossible to distinct when it's about silencing ESLint and when it's about not being interested of the result of the operation.

It creates dangerous precedents in the code base, it's definitely not self-explanatory plus using void promiseFn() is perfect example of "hiding the intent"

I think typescript-eslint should do a better job when explaining the differences and not recommending to use void just to silence the ESLint rule...

@JoshuaKGoldberg
Copy link
Member

@vire that feels like a separate issue to be filed to improve the docs. :)

Always up for that kind of improvement!

@bradzacher
Copy link
Member

As per our contributing guide - please don't comment on old, closed issues. Prefer filing a new issue instead.

@typescript-eslint typescript-eslint locked and limited conversation to collaborators May 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

4 participants