2

I've inherited a codebase which is full of functions like this:

const someFunc = async (): Promise<string> => {
    return new Promise(async (resolve, reject) => {
        try {
            const result = await doSomething();
            resolve(result);
        } catch (error) {
            reject(error);
        }
    });
};

It is my understanding that since the error is not handled in the catch this is essentially the same as doing this:

const someFunc = (): Promise<string> => {
    return doSomething();
};

Did I miss something?

Adam Arold
  • 29,285
  • 22
  • 112
  • 207
  • 2
    see: [What is the explicit promise construction antipattern and how do I avoid it?](https://stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it) – Jamiec Apr 25 '22 at 15:27

1 Answers1

2

This is horrible indeed. Never pass an async function as the executor to new Promise!

Did I miss something?

Synchronous exceptions thrown by doSomething. We assume it returns a promise, so this should never happen, but if they do, then your code is not strictly equivalent to the original which returns a rejected promise. You'd fix this by simply making it an async function:

// eslint-disable-next-line require-await -- async is used to catch the synchronous exceptions
const someFunc = async (): Promise<string> => {
//               ^^^^^
    return doSomething();
};

If this is not an issue, you could shorten the code even further:

const someFunc: () => Promise<string> = doSomething;
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • I know some linter configurations warn when `async` is used without `await`, and also when `return await ...` is used (which I understand is fundamentally different than `return` when used inside a `try` block in order to catch rejections as well as synchronous errors). Do you recommend just disabling `require-await` in ESlint or do you have a workaround you'd suggest? – Patrick Roberts Apr 25 '22 at 18:07
  • 1
    @PatrickRoberts In that case I'd just use `// eslint-disable-next-line require-await -- async is used to catch the synchronous exceptions`. But if `doSomething` is written properly, it won't throw a synchronous exception, so this is not necessary :-) – Bergi Apr 25 '22 at 18:14
  • 1
    I like that, because it emphasizes the fact that the `async` is code-smell because of the sub-par implementation of `doSomething`. – Patrick Roberts Apr 25 '22 at 18:18