-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
fix(eslint-plugin): [no-floating-promises] false negative calling .then with second argument undefined #6881
fix(eslint-plugin): [no-floating-promises] false negative calling .then with second argument undefined #6881
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. |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 822ddc6. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 2 targetsSent with 💌 from NxCloud. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6881 +/- ##
==========================================
+ Coverage 87.54% 87.57% +0.02%
==========================================
Files 377 377
Lines 13172 13201 +29
Branches 3892 3901 +9
==========================================
+ Hits 11532 11561 +29
Misses 1261 1261
Partials 379 379
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congrats on an excellent first open source PR! 🚀
This generally looks good to me, except for a few small touchups - and the big TODO
.
No major (or minor) faux pas 😄 - I appreciate you asking for clarification and adhering to existing styles really well!
Hi @JoshuaKGoldberg, it's been about 3 months since the last activity on this PR, which is the amount of time that the contributing guide suggests to wait before pinging pending reviewers, so I am gently pinging you. Would you be able to take a final pass on this? I believe I have made the minor changes requested. |
Yes! @kirkwaiblinger so sorry about the delay - you've done everything right (and are even being very kind about my blatantly ignoring you 😅). Very sorry to do that. Now that we're done with v6, I'm going through the PR queue. But I shouldn't have let this go without at least an ACK - sorry about that. I'll try to make sure to not leave things open for this long again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK! Again so very sorry about taking so many months to get to this (and on your first PR even! 😭).
The only blocking comment is adding a test case for the ignoreVoid
option, to make sure that error ID logic is tested. Everything else is nitpicking and non-blocking - no worries if you ignore those comments 😄.
If you don't have time to address the changes, no worries - I'll set a reminder for myself to tackle them on Sunday or Monday morning before our next scheduled release.
Thanks again - really great set of PRs here! ❤️
packages/eslint-plugin/tests/rules/no-floating-promises.test.ts
Outdated
Show resolved
Hide resolved
…en with second argument undefined (Issue #6850)
No worries! |
I'm not really sure what caused the CI failures, but neither appears to be directly related to my changes as far as I can tell. Looking for advice on this |
🙃 love it. We might have too many unit tests for the Jest+Node setup we're using. Passed on re-run: https://github.com/typescript-eslint/typescript-eslint/actions/runs/5558453479/jobs/10164457558 & https://github.com/typescript-eslint/typescript-eslint/actions/runs/5558453479/jobs/10164509125. 🤷. If it keeps happening we can file an issue and take a look at reducing the memory footprint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 thanks - excited to land this one!
PR Checklist
Overview
In no-floating-promises, the presence of an
onRejected
handler for.then
or.catch
is one of the valid ways of terminating an expression involving a promise. However, not all values supplied foronRejected
are actually valid. Namely, anything that is not a function (looking at you,null
andundefined
) is simply ignored in EcmaScript. This PR adds validation to the rejection handler to verify that it is actually a function.In short,
In addition to that narrow issue, I have added the same validation for the
.catch
handler, since the same principles applies.In general, any
onRejected
argument to.then
or.catch
is now required to be callable.There is a special case error message per discussion in the issue.
I do have several things to bring to the attention of reviewers
I notice that this rule considers any "Promise-like" thenables, not just EcmaScript Promises. The claim that all rejection handlers are ignored if they are not callable is explicitly based on EcmaScript Promises, and may not be true for other Promise implementations. Is this a concern? Or should we go ahead with flagging errors assuming the ES implementation, despite the fact that, in principle, they could be false positives for an arbitrary thenable?
The rule at present never flags on a
.finally
that has a nonzero number of arguments. I don't actually understand why (is this intentional behavior or a bug??), since finally can make a resolved promise reject, but it can never make a rejected promise resolve. Because I don't understand the intention, and because the finally handler is not a rejection handler, I made no attempt to add validation to the handler in.finally
.I'm not totally happy with passing the result object up the recursion in
isHandledPromise
, but I'm not sure I know of a simpler way of passing the additional info required in order to special case the error message. Open to suggestions...PS this is my first open source PR. Apologies in advance if I commit any major faux pas