feat(eslint-plugin): [no-floating-promises] add an 'allowForKnownSafePromises' option #8502

arka1002 · 2024-02-18T16:37:10Z

PR Checklist

Addresses an existing open issue: fixes Enhancement: [no-floating-promises] add an 'allowForKnownSafePromises' option #7008
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

~~Hi, I'm really curious, is there any way to test code which have import in them ? #4436 also didn't have them.~~
Nvm, found it.

Let's discuss.

'allowForKnownSafePromises' option fixes: typescript-eslint#7008

typescript-eslint · 2024-02-18T16:37:13Z

Thanks for the PR, @arka1002!

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.

netlify · 2024-02-18T16:37:29Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`cbbda8a`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/665304aa7961aa000851b468
😎 Deploy Preview	https://deploy-preview-8502--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 99 (🟢 up 7 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.

nx-cloud · 2024-02-18T16:38:48Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit cbbda8a. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 31 targets

Sent with 💌 from NxCloud.

packages/type-utils/src/TypeOrValueSpecifier.ts

packages/type-utils/typings/typescript.d.ts

packages/eslint-plugin/docs/rules/no-floating-promises.md

packages/type-utils/src/TypeOrValueSpecifier.ts

packages/eslint-plugin/tests/rules/no-floating-promises.test.ts

packages/eslint-plugin/docs/rules/no-floating-promises.md

JoshuaKGoldberg

Haven't been able to look deeply yet, but - looks like there's a mismatch in naming?

arka1002 · 2024-02-20T15:15:25Z

Ahh... I think I made a terrible mistake.

Thanks for the reviews though.
I'm marking this as draft. I need some more time to thoroughly read the issues once again.

Sorry for the inconvenience.

JoshuaKGoldberg · 2024-03-17T16:52:14Z

👋 @arka1002 adding the new allow* options to no-floating-promises is one of our priorities for the next few months. I'd like to make sure this is actively moving towards completion. Do you think you'll be able to make a lot of progress over the next week?

If not, no worries - I'd happily take this on myself. It's a nuanced rule with a lot of context around it. ❤️

arka1002 · 2024-03-17T17:07:36Z

Yes, I've almost done the tests, give me just 3 more days. I'll open it for review.

JoshuaKGoldberg

🆒 thanks for trimming down the test cases! I think this is looking pretty great. Just one last bug from my end and I think I'll be done requesting changes (hopefully, probably).

I'll also leave this open to get at least one other review from @typescript-eslint/triage-team. I've looked at it enough that I don't trust myself to not be missing something. 😄

packages/eslint-plugin/docs/rules/no-floating-promises.mdx

packages/eslint-plugin/tests/rules/no-floating-promises.test.ts

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

packages/eslint-plugin/docs/rules/no-floating-promises.mdx

packages/eslint-plugin/src/rules/no-floating-promises.ts

packages/eslint-plugin/tests/rules/no-floating-promises.test.ts

packages/eslint-plugin/src/rules/no-floating-promises.ts

packages/eslint-plugin/tests/rules/no-floating-promises.test.ts

packages/eslint-plugin/docs/rules/no-floating-promises.mdx

kirkwaiblinger

Requesting some changes in #8502 (comment), but otherwise I think this is getting close! 🙂

arka1002 · 2024-05-11T12:43:13Z

Thanks for the reviews. I'm just writing down some notes for the future people who are going to stumble upon this pr.

then and catch functions with 1 and 0 arguments should be reported based on the above review comment. Here are the tests.
About mixed arrays, its very interesting though, Typescript actually reduces the type of the array if it contains any branded type or anything else. Here is the minimum repro(check the 2 slash comments). I have added 1 test. I think if in any future version typescript changes the type to ( Promise<number> | A )[] ( which might be the correct one ), we have to change the logic in this rule.

kirkwaiblinger · 2024-05-18T20:41:55Z

packages/eslint-plugin/tests/rules/no-floating-promises.test.ts

@@ -2079,19 +2079,19 @@ promise;
    },
    {
      code: `
-interface UnsafeThenable<T> {
+interface SafeThenable<T> {


Nit... this one, should be valid, because the overall expression results in a SafeThenable rather than ordinary promise (due to the way the SafeThenable#then is defined)

Yes, there's an issue which exactly describes Thenable in this rule - #8433. (It also says thenables should be valid)
It'll be solved there.

That is a separate thing entirely (and if it were the case, it would make this entire PR moot, not just this test case). This option needs to be able to support expressions that result in one of the allowed types.

Please move this case to the valid array.

packages/eslint-plugin/tests/rules/no-floating-promises.test.ts

kirkwaiblinger

Hey @arka1002! 👋

Heads up that that this PR is blocking some high-priority no-floating-promises issues. We appreciate how much work you've put into it 🙏 , but we really need to wrap it up soon.

We're going to "time-box" this PR; if it isn't merged before Monday the 3rd, we'll plan to take it over ourselves in order to have it ready for the release ~1pm EST that day.

If you have time to iterate with us this week on the requested changes, that's awesome! If not, we also understand, and we have bandwidth to handle the final rounds of change requests on our side.

Thanks again for your work on this! ❤️

kirkwaiblinger · 2024-05-29T19:46:31Z

packages/eslint-plugin/tests/rules/no-floating-promises.test.ts

@@ -2079,19 +2079,19 @@ promise;
    },
    {
      code: `
-interface UnsafeThenable<T> {
+interface SafeThenable<T> {


That is a separate thing entirely (and if it were the case, it would make this entire PR moot, not just this test case). This option needs to be able to support expressions that result in one of the allowed types.

Please move this case to the valid array.

kirkwaiblinger · 2024-05-29T19:56:56Z

packages/eslint-plugin/src/rules/no-floating-promises.ts

    },
  ],

  create(context, [options]) {
    const services = getParserServices(context);
    const checker = services.program.getTypeChecker();
+    // TODO: #5439
+    // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+    const allowForKnownSafePromises = options.allowForKnownSafePromises!;

    return {
      ExpressionStatement(node): void {


Spinning off from https://github.com/typescript-eslint/typescript-eslint/pull/8502/files/7422cf504ba51cc2e1855f0352e151877720755e#r1585695716, please move the duplicative checks for doesTypeMatchSpecifier throughout the ExpressionStatement handler into the start of isPromiseLike function down below. (this will also resolve the test case in https://github.com/typescript-eslint/typescript-eslint/pull/8502/files/7422cf504ba51cc2e1855f0352e151877720755e#r1605894419).

(You might need to pass in context or just move isPromiseLike into the create function so it has access to the options and such, similarly to how you already handled isPromiseArray 🙂).

feat: [no-floating-promises] add an

476f867

'allowForKnownSafePromises' option fixes: typescript-eslint#7008

arka1002 marked this pull request as draft February 18, 2024 16:49

chore: snapshot errors

7bbba26

arka1002 marked this pull request as ready for review February 18, 2024 17:23

arka1002 commented Feb 18, 2024

View reviewed changes

packages/type-utils/src/TypeOrValueSpecifier.ts Outdated Show resolved Hide resolved

arka1002 commented Feb 18, 2024

View reviewed changes

packages/type-utils/typings/typescript.d.ts Outdated Show resolved Hide resolved

arka1002 changed the title ~~feat: [no-floating-promises] add an 'allowForKnownSafePromises' option~~ feat(eslint-plugin): [no-floating-promises] add an 'allowForKnownSafePromises' option Feb 19, 2024

StyleShit suggested changes Feb 19, 2024

View reviewed changes

arka1002 added 2 commits February 19, 2024 23:24

chore: add a test

38ea99f

chore: add another test

f15a0bb

JoshuaKGoldberg reviewed Feb 20, 2024

View reviewed changes

packages/eslint-plugin/docs/rules/no-floating-promises.md Outdated Show resolved Hide resolved

JoshuaKGoldberg requested changes Feb 20, 2024

View reviewed changes

arka1002 marked this pull request as draft February 20, 2024 15:15

This was referenced Feb 22, 2024

Enhancement: [no-floating-promises] add an 'allowForKnownSafePromises' option #7008

Open

Bug: [readonly-parameter-types] allow option does not support Node.js builtin modules #8572

Open

arka1002 added 5 commits March 18, 2024 21:45

chore: rewrite

0019678

Merge branch 'main' into issue-7008

d231660

chore: docs & test

379a2d0

chore: add jsdoc comment

035484b

chore: cspell

6fe2efa

arka1002 marked this pull request as ready for review March 19, 2024 14:25

arka1002 requested review from JoshuaKGoldberg and StyleShit March 19, 2024 14:39

chore: lint

2269319

arka1002 requested a review from JoshuaKGoldberg April 27, 2024 16:16

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Apr 27, 2024

JoshuaKGoldberg requested changes Apr 28, 2024

View reviewed changes

packages/eslint-plugin/docs/rules/no-floating-promises.mdx Outdated Show resolved Hide resolved

packages/eslint-plugin/tests/rules/no-floating-promises.test.ts Show resolved Hide resolved

arka1002 and others added 2 commits April 29, 2024 13:09

chore: update docs

171701b

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

chore: tag function

ecd31e3

arka1002 requested a review from JoshuaKGoldberg April 29, 2024 11:21

JoshuaKGoldberg requested review from a team and removed request for StyleShit April 29, 2024 12:25

kirkwaiblinger requested changes May 1, 2024

View reviewed changes

arka1002 added 2 commits May 1, 2024 15:31

chore: address reviews

a7fc9a2

chore: revert var name

0ab7060

arka1002 requested a review from kirkwaiblinger May 1, 2024 12:47

kirkwaiblinger added awaiting response Issues waiting for a reply from the OP or another party and removed awaiting response Issues waiting for a reply from the OP or another party labels May 1, 2024

kirkwaiblinger requested changes May 8, 2024

View reviewed changes

arka1002 added 2 commits May 11, 2024 16:57

chore: address reviews

441e467

chore: lint & tests

7422cf5

arka1002 requested a review from kirkwaiblinger May 11, 2024 12:44

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label May 11, 2024

kirkwaiblinger requested changes May 18, 2024

View reviewed changes

kirkwaiblinger added the awaiting response Issues waiting for a reply from the OP or another party label May 18, 2024

chore: comments

cbbda8a

arka1002 requested a review from kirkwaiblinger May 26, 2024 10:01

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label May 26, 2024

bradzacher added the enhancement: plugin rule option New rule option for an existing eslint-plugin rule label May 28, 2024

kirkwaiblinger requested changes May 29, 2024

View reviewed changes

arka1002 closed this May 30, 2024

kirkwaiblinger mentioned this pull request May 30, 2024

fix(eslint-plugin): [no-floating-promises] add option 'allowForKnownSafePromises' #9186

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): [no-floating-promises] add an 'allowForKnownSafePromises' option #8502

feat(eslint-plugin): [no-floating-promises] add an 'allowForKnownSafePromises' option #8502

arka1002 commented Feb 18, 2024 •

edited by JoshuaKGoldberg

typescript-eslint bot commented Feb 18, 2024

netlify bot commented Feb 18, 2024 •

edited

nx-cloud bot commented Feb 18, 2024 •

edited

JoshuaKGoldberg left a comment

arka1002 commented Feb 20, 2024

JoshuaKGoldberg commented Mar 17, 2024

arka1002 commented Mar 17, 2024

JoshuaKGoldberg left a comment

kirkwaiblinger left a comment

arka1002 commented May 11, 2024

kirkwaiblinger May 18, 2024

arka1002 May 26, 2024 •

edited

kirkwaiblinger May 29, 2024 •

edited

kirkwaiblinger left a comment

kirkwaiblinger May 29, 2024 •

edited

kirkwaiblinger May 29, 2024

feat(eslint-plugin): [no-floating-promises] add an 'allowForKnownSafePromises' option #8502

feat(eslint-plugin): [no-floating-promises] add an 'allowForKnownSafePromises' option #8502

Conversation

arka1002 commented Feb 18, 2024 • edited by JoshuaKGoldberg

PR Checklist

Overview

typescript-eslint bot commented Feb 18, 2024

netlify bot commented Feb 18, 2024 • edited

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Feb 18, 2024 • edited

☁️ Nx Cloud Report

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

arka1002 commented Feb 20, 2024

JoshuaKGoldberg commented Mar 17, 2024

arka1002 commented Mar 17, 2024

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

kirkwaiblinger left a comment

Choose a reason for hiding this comment

arka1002 commented May 11, 2024

kirkwaiblinger May 18, 2024

Choose a reason for hiding this comment

arka1002 May 26, 2024 • edited

Choose a reason for hiding this comment

kirkwaiblinger May 29, 2024 • edited

Choose a reason for hiding this comment

kirkwaiblinger left a comment

Choose a reason for hiding this comment

kirkwaiblinger May 29, 2024 • edited

Choose a reason for hiding this comment

kirkwaiblinger May 29, 2024

Choose a reason for hiding this comment

arka1002 commented Feb 18, 2024 •

edited by JoshuaKGoldberg

netlify bot commented Feb 18, 2024 •

edited

nx-cloud bot commented Feb 18, 2024 •

edited

arka1002 May 26, 2024 •

edited

kirkwaiblinger May 29, 2024 •

edited

kirkwaiblinger May 29, 2024 •

edited