Skip to content

Conversation

marksvc
Copy link
Collaborator

@marksvc marksvc commented Aug 21, 2025

The rules are not enabled for spec and stories files. It may also be
useful to have the rules on in those files, but (1) calls to async
methods in fakeAsync do not need awaited, and (2) the IDE understands
toBe, toEqual, etc to return Promises.

Ideally the rules would mark occurrences as errors, but there are many
occurrences that first need to be cleaned up.


This change is Reviewable

@marksvc
Copy link
Collaborator Author

marksvc commented Aug 21, 2025

This PR turns on eslint warnings for rules no-floating-promises and no-misused-promises.

Consider this code:

async myMethod(): Promise<void> {
  mkdir('/data');
  await writeFile('/data/hello.txt', 'Hello, world!');
}

The call to async function mkdir might not finish before the call to writeFile starts. Rule "no-floating-promises" flags this so you notice that you should await mkdir.

Or consider this code,

async function risk(): Promise<void> {
  throw new Error("Problem!");
}

async function click(): Promise<void> {
  try {
    console.log("Trying something!");
    risk();
  } catch (e) {
    console.log("It was a problem!");
  }
  console.log("Phew!");
}
await click();

When this code runs, will it print "It was a problem!"? Nope! You need to await risk and then the exception will be handled.

Consider this code:

async click(): Promise<void> {
  navigate('/destination');
}

Some calls to async methods might not be important to await. You fire it and it will happen when (and if) it happens. You can prefix void to the call to specify that you intend to call an async method, without awaiting it, and without handling any exceptions that happen. For example, void navigate('/destination');

Our codebase has a lot of these occurrences, so I am having eslint mark them as warnings rather than errors.

Copy link

codecov bot commented Aug 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.59%. Comparing base (4d3a87c) to head (76c0c54).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3391   +/-   ##
=======================================
  Coverage   82.59%   82.59%           
=======================================
  Files         608      608           
  Lines       35966    35966           
  Branches     5842     5842           
=======================================
  Hits        29705    29705           
  Misses       5399     5399           
  Partials      862      862           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@marksvc marksvc marked this pull request as ready for review August 21, 2025 22:38
The rules are not enabled for spec and stories files. It may also be
useful to have the rules on in those files, but (1) calls to async
methods in fakeAsync do not need awaited, and (2) the IDE understands
toBe, toEqual, etc to return Promises.

Ideally the rules would mark occurrences as errors, but there are many
occurrences that first need to be cleaned up.
@pmachapman pmachapman self-requested a review August 24, 2025 19:41
@pmachapman pmachapman self-assigned this Aug 24, 2025
Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

So many warnings!

The warnings I investigated looked like they were intended by the developer to be floating or misused.

I'd be interested in getting wider consensus/feedback form the team before merging. Should we discuss this in the planning meeting tomorrow?

@pmachapman reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @marksvc)

@marksvc
Copy link
Collaborator Author

marksvc commented Aug 27, 2025

Sounds good.

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.

2 participants