-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin): [no-misused-promises] check more places for checksVoidReturn #4541
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
Conversation
Thanks for the PR, @uhyo! 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. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
❌ Deploy Preview for typescript-eslint failed. 🔨 Explore the source changes: 42701f5 🔍 Inspect the deploy log: https://app.netlify.com/sites/typescript-eslint/deploys/6217b76f95b0d100079e1af4 |
Codecov Report
@@ Coverage Diff @@
## main #4541 +/- ##
==========================================
+ Coverage 94.62% 94.65% +0.03%
==========================================
Files 149 150 +1
Lines 8072 8162 +90
Branches 2581 2617 +36
==========================================
+ Hits 7638 7726 +88
- Misses 239 241 +2
Partials 195 195
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.
Generally looks great to me so far, thanks for tackling all these cases @uhyo!
Requesting changes on the error message I commented on. Also, codecov (now that it's done changing its mind) is correct, there are some untested lines of code. Please do fill those in.
Co-authored-by: Josh Goldberg <me@joshuakgoldberg.com>
@JoshuaKGoldberg Thank you for review! |
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.
Sorry, one more bug? 😄
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 for the improvement @uhyo ! I'm wondering, is there a way to disable only the function MyButton() {
const onClick = async () => {
const b = await doSomething();
doSomethingElse(b);
}
return <button onClick={onClick}>Click me!</button>;
} and now that error is being (correctly!) detected on those places. I know I could do |
As per our contributing guide - please don't comment on closed PRs. Instead please use issues to start new discussions and find existing ones like #4619 or #4609 |
PR Checklist
Overview
The
checksVoidReturn: true
option only checks function arguments now. This PR extends the rule so that misused Promises at other places are checked too. Namely, variable assignments, object properties, return statements and JSX props are newly checked.See the added tests for examples of newly-incorrect code.