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 Eslint rule to handle promises #6005

Open
milanholemans opened this issue Apr 29, 2024 · 3 comments
Open

Add Eslint rule to handle promises #6005

milanholemans opened this issue Apr 29, 2024 · 3 comments

Comments

@milanholemans
Copy link
Contributor

milanholemans commented Apr 29, 2024

Contributors and maintainers are humans and they make errors. Sometimes we merge stuff that we overlooked and have to fix later on. One thing I noticed is that we sometimes forget that logging a message is a promise. It used to be a void function, but since September last year, it has become a promise.
Sometimes messages are logged without being awaited, which results in undesired behavior. In fact, in almost all cases, we should always handle promises correctly. This is something that can be easily fixed by enabling an ESlint rule.

Goal

Configure ESlint rules to ensure that all promises are handled correctly.

Info

Something we can look into is the ESlint rule no-floating-promises. Let's check if this satisfies our needs.

Maybe it's also worth checking the no-misused-promises rule as extra.

@milanholemans milanholemans added enhancement needs peer review Needs second pair of eyes to review the spec or PR labels Apr 29, 2024
@Jwaegebaert
Copy link
Contributor

Good one, @milanholemans! That's something that can be easily overlooked, so an ESLint rule for this could go a long way.

@Jwaegebaert Jwaegebaert added help wanted and removed needs peer review Needs second pair of eyes to review the spec or PR labels Apr 30, 2024
@waldekmastykarz
Copy link
Member

Good point! I vaguely recall looking at it but dismissing it at the end. Unfortunately, I don't know exactly why. My gut feeling tells me something wasn't working as expected, but let's double check. Maybe things have changed and we can cover this issue just fine now.

@MathijsVerbeeck
Copy link
Contributor

Just quickly jumping in here, as we are still in the process of merging #5837 , in which there were quite a few floating promises (mostly in the Command.ts file, where many logs werent being awaited), so could be handy if we wait until this PR is merged to avoid conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants