-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Enhancement: [no-floating-promises] Check PromiseLike in addition to Promise by default #9869
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
For reference, now that function isPromiseLike(checker: ts.TypeChecker, type: ts.Type): boolean {
const awaited = checker.getAwaitedType(type);
return !!awaited && awaited !== type;
} That would have caught all cases where TS thinks something is awaitable (unwrapping nested promises and so on), which would have fixed this issue.
|
+1 |
Doesn't the fastify API use |
It does, #8433 (comment) -> https://github.com/fastify/fastify/blob/dda569960ef2d0961a887ead0e63f4e8671a920e/types/instance.d.ts#L173. But that's just a convenience type - they could always copy just the relevant bits out of it. I believe @mcollina is out this week and maybe the next, we should ping again the first week of September. |
I'm happy for whatever solution that allows us to declare a fire-and-forget promise in a way that does not require a line-by-line or type-by-type configuration and very little user intervention. Setting a config would be fine, even if it would be better if this is enabled by default. From a practical perspective, any kind of solution that would allow this would work. Note that fire-and-forget promises are part of the language and their design. On thenables, my opinion is that a The important part is that you can declare a |
Is there a known benefit to making this change in terms of use cases where the rule would flag, rather than implementation simplification on our side? While the idea of flagging |
(i take back my +1 and am back to undecided, since I didn't recall that the fastify case was |
👍 I too would like to see a use case for checking |
I'm also +0. Either treat all thenables as promises or don't. |
I personally would love for us to do this and simplify our logic to better align things with TS -- but it's just a hard sell for us to make this switch whilst there are known usecases in the community where we don't want to report on a "thenable" thing (eg fastify, node:test). With that being said -- we could probably apply this change to the following without anyone being upset
typescript-eslint/packages/eslint-plugin/src/rules/no-misused-promises.ts Lines 611 to 656 in d4f6943
|
Chatted internally with the team. tl;dr: we're going to wontfix this for For visibility, summarizing our perspective and conversations such as #8433 (comment) after the ---... We're not super enthused about intentionally building in such an escape hatch to the rule. The userland convention most of the time for The tricky thing is, there are a few frameworks -Fastify, This presents a problem for linters such as typescript-eslint. We definitely can't turn My 🌶 hot take is that the best situation would have been for frameworks not to co-opt the But, assuming the API must use "fire-and-forget"
Separately: I've also filed #10666 and commented #10666 (comment) for the occasional request for typescript-eslint to provide its own |
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
As of #8433 -> #9263,
no-floating-promises
has acheckThenables
option (enabled by default as of v8) that stops it from flagging anything that's not explicitly aPromise
type. That means the rule won't flag something that's explicitly marked as typePromiseLike
(rather thanPromise
) without explicitly enablingcheckThenables
.Coming out of conversation with @jakebailey:
PromiseLike
does semantically indicate a value is intended to be used like aPromise
. That's different from something that coincidentally has a.then()
which can be used withawait
(🤯).Proposal: should we have the rule also check for
PromiseLike
, in addition toPromise
?Fail
Pass
Additional Info
I'm in favor of this personally, for the stated reasoning.
In terms of whether this would be a breaking change: it almost feels like a bugfix rather than a feature?
PromiseLike
is quite rare in real-world usage. If this is accepted, I'd also be in favor of landing it in a minor version rather than a major.💖
The text was updated successfully, but these errors were encountered: