Skip to content
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

feat(eslint-plugin): add new rule no-misused-promises #612

Merged
merged 9 commits into from
Jul 16, 2019

Conversation

princjef
Copy link
Contributor

@princjef princjef commented Jun 13, 2019

Adds a new no-misused-promises rule as discussed previously in #508. Fixes #508

Some notes about the implementation:

  • Trying to determine the type of the parameter for which an argument is provided was harder than I expected. The only way I could find to make it work was to enumerate the call signatures of the function being called and matching argument and parameter by index. This can potentially be problematic for functions with many overloads or rest params, but the rule is conservative enough that there should be no false positives in such cases.

  • I ended up making the conditional check apply to any situation where the truthiness/falsiness check could end up swallowing a promise. I can't think of any valid use cases where the checks that I have placed will trigger, but I did carve out a case so that code like const foo = someCheck || promise; won't be flagged due to short-circuit evaulation.

  • There was no entry in RuleListener for LogicalExpression, so I added it to the type and everything seemed to work. I'm assuming this was just an omission from the type.

@codecov
Copy link

codecov bot commented Jun 26, 2019

Codecov Report

Merging #612 into master will increase coverage by 0.09%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #612      +/-   ##
==========================================
+ Coverage   94.33%   94.43%   +0.09%     
==========================================
  Files         111      112       +1     
  Lines        4593     4672      +79     
  Branches     1268     1286      +18     
==========================================
+ Hits         4333     4412      +79     
  Misses        149      149              
  Partials      111      111
Impacted Files Coverage Δ
...ges/eslint-plugin/src/rules/no-misused-promises.ts 100% <100%> (ø)
packages/eslint-plugin/src/rules/index.ts 100% <100%> (ø) ⬆️

packages/eslint-plugin/src/rules/no-misused-promises.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/no-misused-promises.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/no-misused-promises.ts Outdated Show resolved Hide resolved
@bradzacher bradzacher added the enhancement: new plugin rule New rule request for eslint-plugin label Jun 28, 2019
JamesHenry
JamesHenry previously approved these changes Jul 12, 2019
Copy link
Member

@JamesHenry JamesHenry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Thanks for doing this

@JamesHenry JamesHenry added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Jul 12, 2019
bradzacher
bradzacher previously approved these changes Jul 16, 2019
@bradzacher
Copy link
Member

awesome! great work - thanks for the contribution.

@princjef princjef dismissed stale reviews from bradzacher and JamesHenry via f321ced July 16, 2019 06:09
@twavv
Copy link

twavv commented Dec 13, 2019

Related (but a few months late):
I've opened this issue on TypeScript itself to make using a promise in a condition a compiler error (microsoft/TypeScript#34717) à la the change in 3.7 to make non-nullable functions in conditions an error.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge enhancement: new plugin rule New rule request for eslint-plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule proposal: no-misused-promises
4 participants