fix(eslint-plugin): [no-floating-promises] finally should be transparent to unhandled promises #7092
Conversation
|
Thanks for the PR, @kirkwaiblinger! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #7092 +/- ##
=======================================
Coverage 87.47% 87.47%
=======================================
Files 379 379
Lines 13230 13230
Branches 3906 3906
=======================================
Hits 11573 11573
Misses 1279 1279
Partials 378 378
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
This looks great, thanks a bunch @kirkwaiblinger!
Sorry for taking so long to review 😞 we've been caught up in v6 work for a while and I'm only now getting back into the PR review queue.
Approving now, will look at the other PR too.
| @@ -191,12 +184,8 @@ async function test() { | |||
| () => {}, | |||
There was a problem hiding this comment.
[Non-actionable] Aside: the promiseValue.finally(() => {}); on line 173 (unchanged by this PR) can be removed because that test case doesn't define a promiseValue value. It's an existing typo 😄. Not requesting any action, just noting for reference. This is good good first issue fodder.
| if (isPromiseFinallyCallWithHandler(node)) { | ||
| return { isUnhandled: false }; | ||
| // `x.finally()` is transparent to resolution of the promise, so check `x`. | ||
| // ("object" in this context is the `x` in `x.finally()`) |
There was a problem hiding this comment.
[Praise] This second line was very helpful for understanding the code :) great comment!
PR Checklist
Overview
The existing implementation treats
.finallyas though it can account for a rejected promise. It cannot prevent rejection. This PR implements the change recommended in the discussion for this issue, namely, to make.finallytransparent to the rule and check whatever comes before it.Note - this will have conflicts with #6881. It would be simpler to merge that PR first and then deal with the conflicts here.