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): [prefer-promise-reject-errors] add rule #8011

Conversation

auvred
Copy link
Member

@auvred auvred commented Dec 1, 2023

PR Checklist

Overview

I tried to migrate the original rule with only type information added (it allows to detect PromiseConstructor like and Error like variables)

Also I moved isErrorLike function to the type-utils package (I hope it's the right place for this function) as its logic may be reused both in prefer-promise-reject-errors and no-throw-literal

I copied incorrect and correct examples from no-throw-literal because I found them applicable. But maybe there are too many examples for this rule

@typescript-eslint
Copy link
Contributor

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 on https://opencollective.com/typescript-eslint.

Copy link

netlify bot commented Dec 1, 2023

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit bd20782
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/659a6189a92e2f00080aa1f9
😎 Deploy Preview https://deploy-preview-8011--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 90 (🔴 down 9 from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 98 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@auvred auvred marked this pull request as draft December 1, 2023 15:16
@auvred auvred marked this pull request as ready for review December 2, 2023 09:04
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a 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 to Thenable.

A few touchups & refactors requested, but the general direction looks great to me! 🚀

Comment on lines 72 to 86
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;
}
Copy link
Member

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 as isErrorLike's for checking if it's a built-in of a particular name. Could you extract into a shared helper for this too?

Copy link
Member Author

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) {
const declarations = symbol.getDeclarations() ?? [];
for (const declaration of declarations) {
const sourceFile = declaration.getSourceFile();
if (services.program.isSourceFileDefaultLibrary(sourceFile)) {
return true;
}
}
}
if (symbol) {
const declarations = symbol.getDeclarations() ?? [];
for (const declaration of declarations) {
const sourceFile = declaration.getSourceFile();
if (services.program.isSourceFileDefaultLibrary(sourceFile)) {
context.report({ node, messageId: 'noFunctionConstructor' });
return;
}
}

Copy link
Member

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 a good first issue followup. Either way.

Copy link
Member Author

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 with good first issue ;)

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Dec 5, 2023
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a 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!

@JoshuaKGoldberg JoshuaKGoldberg added the 1 approval One team member has approved this PR; a second should be enough to merge it label Dec 29, 2023
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
@auvred auvred force-pushed the feat/prefer-promise-reject-errors-rule branch from ad4dc75 to 895f1b5 Compare December 29, 2023 19:46
Tjstretchalot added a commit to Tjstretchalot/typescript-eslint that referenced this pull request Jan 2, 2024
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a 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.

@JoshuaKGoldberg JoshuaKGoldberg added awaiting response Issues waiting for a reply from the OP or another party and removed 1 approval One team member has approved this PR; a second should be enough to merge it labels Jan 4, 2024
@github-actions github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Jan 4, 2024
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a 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. 🚀

Neil Patrick Harris giving two thumbs up and grinning

@JoshuaKGoldberg JoshuaKGoldberg merged commit 1aa8664 into typescript-eslint:main Jan 9, 2024
57 checks passed
Tjstretchalot added a commit to Tjstretchalot/typescript-eslint that referenced this pull request Jan 10, 2024
Tjstretchalot added a commit to Tjstretchalot/typescript-eslint that referenced this pull request Jan 12, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Base rule extension: [prefer-promise-reject-errors]
3 participants