Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
feat(eslint-plugin): [prefer-promise-reject-errors] add rule#8011
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
feat(eslint-plugin): [prefer-promise-reject-errors] add rule#8011
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for the PR,@auvred! 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 onhttps://opencollective.com/typescript-eslint. |
netlifybot commentedDec 1, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify site configuration. |
packages/eslint-plugin/docs/rules/prefer-promise-reject-errors.md OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
packages/eslint-plugin/tests/rules/prefer-promise-reject-errors.test.ts OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
packages/eslint-plugin/tests/rules/prefer-promise-reject-errors.test.ts OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
packages/eslint-plugin/tests/rules/prefer-promise-reject-errors.test.ts OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
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.
Awesome, really solid work here - thanks for working on it@auvred!
It's interesting that this is the first rule to really need to check whether something is actually a'PromiseConstructor'. Others have just approximated comparisons toThenable.
A few touchups & refactors requested, but the general direction looks great to me! 🚀
packages/eslint-plugin/docs/rules/prefer-promise-reject-errors.md OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| function isPromiseConstructorLike(type: ts.Type): boolean { | ||
| const symbol = type.getSymbol(); | ||
| if (symbol?.getName() === 'PromiseConstructor') { | ||
| const declarations = symbol.getDeclarations() ?? []; | ||
| for (const declaration of declarations) { | ||
| const sourceFile = declaration.getSourceFile(); | ||
| if (program.isSourceFileDefaultLibrary(sourceFile)) { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| return false; | ||
| } |
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.
[Refactor] This is roughly the same logic asisErrorLike's for checking if it's a built-in of a particular name. Could you extract into a shared helper for this too?
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.
Not sure about the function name, but I extracted it:https://github.com/typescript-eslint/typescript-eslint/pull/8011/files#diff-75626015398ba4cc6293d6ec23077170a50f23e306b326a44966e44448c3edf2
TODO: refactor these places to use this function
| if(symbol&&symbol.escapedName===FUNCTION_CONSTRUCTOR){ | |
| constdeclarations=symbol.getDeclarations()??[]; | |
| for(constdeclarationofdeclarations){ | |
| constsourceFile=declaration.getSourceFile(); | |
| if(services.program.isSourceFileDefaultLibrary(sourceFile)){ | |
| returntrue; | |
| } | |
| } | |
| } |
typescript-eslint/packages/eslint-plugin/src/rules/no-implied-eval.ts
Lines 145 to 153 in705370a
| if(symbol){ | |
| constdeclarations=symbol.getDeclarations()??[]; | |
| for(constdeclarationofdeclarations){ | |
| constsourceFile=declaration.getSourceFile(); | |
| if(services.program.isSourceFileDefaultLibrary(sourceFile)){ | |
| context.report({ node,messageId:'noFunctionConstructor'}); | |
| return; | |
| } | |
| } |
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.
LGTM! If you want to do that refactor in this PR I'd happily approve. Or we can leave it as agood first issue followup. Either way.
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.
Let's open a followup withgood first issue ;)
Uh oh!
There was an error while loading.Please reload this page.
packages/eslint-plugin/tests/rules/prefer-promise-reject-errors.test.ts OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
This looks lovely, thanks! 🙌
No blocking requests (other than the"guarantee" nit) from me. Just some touchup suggestions. Feel free to wontfix or do them, and then we can merge. Or if you don't have time I'll take another look soon. Cheers!
packages/eslint-plugin/docs/rules/prefer-promise-reject-errors.md OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
ad4dc75 to895f1b5CompareThere 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, two blocking requests:
- The variable readability change actually introduced a performance regression - lmk if the suggestion doesn't make sense?
- Apologies, I missed commenting on a couple of missing test cases
Since I already approved I do feel bad now requesting changes. If you don't have energy+time then no worries. I can apply these soon.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Super, thanks! This looks great. 🚀
PR Checklist
Overview
I tried to migrate theoriginal rule with only type information added (it allows to detect
PromiseConstructorlike andErrorlike variables)Also I moved
isErrorLikefunction to thetype-utilspackage (I hope it's the right place for this function) as its logic may be reused both inprefer-promise-reject-errorsandno-throw-literalI copied incorrect and correct examples from
no-throw-literalbecause I found them applicable. But maybe there are too many examples for this rule