matrix-js-sdk icon indicating copy to clipboard operation
matrix-js-sdk copied to clipboard

Enable `no-floating-promises` and `no-misused-promises`

Open ShadowJonathan opened this issue 4 years ago • 2 comments

https://typescript-eslint.io/rules/no-floating-promises/

https://typescript-eslint.io/rules/no-misused-promises

This'd disallow not handling values and errors on promises, and not awaiting promises themselves.

ShadowJonathan avatar Jan 23 '22 14:01 ShadowJonathan

Why?

turt2live avatar Jan 24 '22 18:01 turt2live

First, this would make sure that promises aren't missed, and that await chains are explicitly broken.

Example;

declare async function otherThing(): number;

async function thing(): number {
  otherThing() // not returned or awaited
}

In this case, no-floating-promises would make sure that above promise is either explicitly awaited, or returned.

It will also enforce that all control flow options in callback-based promise chaining are resolved, and not dropped.

Example, from this;

returnsPromise().then(() => {});

Promise.reject('value').catch();

Promise.reject('value').finally();

To this;

returnsPromise().then(
  () => {},
  () => {},
);

Promise.reject('value').catch(() => {});

Promise.reject('value').finally(() => {});

This'll enforce more correctness and explicitness in working with async functions and promises, making sure no branches are silently dropped.

(I recall one case where a promise was explicitly dropped and left to run on its own, with a comment stating so. If/when the library gets converted to use this linter, that comment can be updated with @ts-ignore to make the ignore more explicit (or possibly with @ts-expect-error (https://github.com/matrix-org/matrix-js-sdk/issues/2122))

Secondly, this'll make sure that promises arent misused as anything but promises, such as implicitly using them in boolean truthiness;

const promise = Promise.resolve('value');

if (promise) {
  // Do something
}

The above would fail with this lint, and (await promise) would be recommended as an alternative.

Furthermore, this'll make sure that promises aren't "tossed around", it'll error on the following;

[1, 2, 3].forEach(async value => {
  await doSomething(value);
});

Alternative and more correct snippets to the above would be;

for (const value of [1, 2, 3]) {
  await doSomething(value);
}

Or, when context isnt async;

Promise.all(
  [1, 2, 3].map(async value => {
    await doSomething(value);
  }),
).catch(() => {/* handle error */});

These two lints will help avoid subtle bugs with promises, and require programmers to be explicit when they are not chaining a promise within an async context. This'll help enforce more correctness, and eliminate a class of hard-to-debug asynchronous execution bugs.

ShadowJonathan avatar Jan 24 '22 22:01 ShadowJonathan