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

Add no-floating-promises @typescript-eslint rule #2452

Closed
lksnmnn opened this issue Jun 4, 2021 · 8 comments
Closed

Add no-floating-promises @typescript-eslint rule #2452

lksnmnn opened this issue Jun 4, 2021 · 8 comments
Labels
good first issue Good for newcomers kind/feature-change New feature or request status/done
Projects

Comments

@lksnmnn
Copy link

lksnmnn commented Jun 4, 2021

What do you want and why?

Adding the above mentioned rule, would catch floating promises and thus helps devs like me to not forget to await my promises.

Discussion:

The ESLint rule require-await does something similar but cannot gather insights about functions imported from other files and thus flags false positives like:

const foo = async () => {
    return bar()
}

Possible implementation(s)

"@typescript-eslint/eslint-plugin": 
"@typescript-eslint/parser": 

Additional context

Add any other context or screenshots about the feature request here.

@blitzjs-bot blitzjs-bot added this to Triage in Dashboard Jun 4, 2021
@flybayer
Copy link
Member

flybayer commented Jun 4, 2021

Probably it's good to add that one, but to prevent issues where you forget to await a promise, we actually need @typescript-eslint/no-floating-promises which requires using typescript as a parser.

We are currently using that in the blitz monorepo like so: https://github.com/blitz-js/blitz/blob/canary/.eslintrc.js#L55-L66

Probably it's a good idea to add to our blitz eslint config for users. It will add two new dependencies,

"@typescript-eslint/eslint-plugin": 
"@typescript-eslint/parser": 

@flybayer flybayer added the kind/feature-change New feature or request label Jun 4, 2021
@Roesh
Copy link
Collaborator

Roesh commented Jun 4, 2021

I think this definitely should be set to warn

@lksnmnn
Copy link
Author

lksnmnn commented Jun 6, 2021

I've now looked into this and maybe I should rewrite this issue to reflect that we need to add the no-floating-promises rule rather than require-await. Because with the latter rule, this gets flagged:

const foo = async () => {
    return bar()
}

and changing this to return await bar() is discouraged my the no-return-await rule, since it adds a small perfomance hit.

@flybayer
Copy link
Member

flybayer commented Jun 7, 2021

@lksnmnn sounds good to me to update the issue

@lksnmnn lksnmnn changed the title Add require-await ESLint rule Add no-floating-promises @typescript-eslint rule Jun 9, 2021
@lksnmnn
Copy link
Author

lksnmnn commented Jun 9, 2021

Updated the issue to reflect our discussion.

@flybayer flybayer added the status/ready-to-work-on This issue is up for grabs label Jun 9, 2021
@blitzjs-bot blitzjs-bot moved this from Triage to Ready to Work On in Dashboard Jun 9, 2021
@flybayer flybayer added the good first issue Good for newcomers label Jun 9, 2021
@flybayer
Copy link
Member

flybayer commented Jun 9, 2021

For whoever wants to work on this, our eslint config is here: https://github.com/blitz-js/blitz/tree/canary/packages/eslint-config You'll need to add the correct dependencies to that package as well as the config file.

@satya-nutella
Copy link
Collaborator

@flybayer Number 5? Can I hop on this one?

@flybayer
Copy link
Member

@meehawk sorry for the delay, of course! Btw, you don't have to wait for confirmation if it's labeled status/ready-to-work-on

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers kind/feature-change New feature or request status/done
Projects
No open projects
Dashboard
Ready to Work On
Development

Successfully merging a pull request may close this issue.

5 participants