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

ESLint rule to disallow "floating" promises #1086

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

fredrikekelund
Copy link
Contributor

@fredrikekelund fredrikekelund commented Mar 18, 2025

Related issues

  • Fixes #

Proposed Changes

  • Enable the @typescript-eslint/no-floating-promises ESLint rule (docs) and fix the places ESLint yells at. So far, I've only fixed a subset of errors. I'm looking for alignment on the team before proceeding.

This rule generates the following error:

Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator

Testing Instructions

CI should pass

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@fredrikekelund fredrikekelund requested a review from a team March 18, 2025 12:25
@fredrikekelund fredrikekelund self-assigned this Mar 18, 2025
// Open external links in the default browser
aboutWindow.webContents.setWindowOpenHandler( ( { url } ) => {
shellOpenExternalWrapper( url );
void shellOpenExternalWrapper( url );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

void is a way of intentionally signaling that no error handling is required for this promise. Most of the linter errors in our existing codebase can be handled this way, and it's the part I expect to be most controversial.

@nightnei
Copy link
Contributor

In general, it's a great idea and a way to improve our codebase, but I have a concern. Let's imagine a case:

const qqq = async () => {
  try {
  
  } catch () {
  	// we handle errors here, so should we force handling errors inside `eee()`. Ofc, we can use `void` inside `eee`, but it looks wordy, however on the other hand it's pretty straightforward.... 
  	}
};
const eee = async () => {
	await qqq();
}

It's mostly thought out loud 😅 Hm, maybe we indeed could try to go this way with ESLint and adding void when it's necessary.

@fredrikekelund
Copy link
Contributor Author

@nightnei, yeah, I know… Having to prefix promises we know won't ever throw an error with void is pretty verbose.

I paused my work on this the other day because I had a slight feeling that 95% of promises would be prefixed with void, which seemed very verbose.

I still think it's fundamentally smart to enforce some kind of error handling for promises. I guess the risk is that we just get into a habit of prefixing everything with void, even promises that should really have a .catch handler…

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

Successfully merging this pull request may close these issues.

3 participants