eslint-plugin-unicorn icon indicating copy to clipboard operation
eslint-plugin-unicorn copied to clipboard

Use `Promise.withResolvers` instead of `new Promise()`?

Open fisker opened this issue 1 year ago • 11 comments

I think in most cases need a promise, using Promise.withResolvers() should be more readable than new Promise(), because new Promise needs a new function scope.

I opened this issue instead of a rule proposal, since I'm not sure about it and want to discuss it first.

Welcome to post cases where new Promise() should be preferred.

The only case in my mind is the arrow function body.

const foo = () => new Promise(resolve => {
  resolve()
});

fisker avatar Jul 22 '24 07:07 fisker

The benefit of new Promise() is that it makes it harder to make mistakes. For example, it's easier to cause an unhandled rejection with Promise.withResolvers() or accidentally synchronously throw in a function that is supposed to be async.

new Promise(resolve => {
	functionThatPotentiallyThrows(); // If this throws, it will cause a proper rejection.
});

sindresorhus avatar Jul 22 '24 10:07 sindresorhus

I think it depends, new Promise() helps encapsulate the logic and I think it makes sense when you can do so. Promise.withResolvers only becomes useful when the logic is too complex.

A lot of p-defer usage I see is rather smelly and lacks encapsulation.

fregante avatar Jul 25 '24 07:07 fregante

@sindresorhus you never use p-defer? 🤔

https://github.com/search?q=org%3Asindresorhus+p-defer&type=code

fregante avatar Jul 25 '24 07:07 fregante

@fregante I'm not saying Promise.withResolvers is not useful, but this issue is asking to prefer Promise.withResolvers, and I'm arguing that new Promise is a better default.

sindresorhus avatar Jul 25 '24 12:07 sindresorhus

We're saying the same thing: prefer new Promise(), and I notice you do to (judging by the lack of use of p-defer in your own modules)

fregante avatar Jul 25 '24 13:07 fregante

I misunderstood your statement. I used to use p-defer more before, but transitioned to new Promise for the reasons I mentioned.

sindresorhus avatar Jul 25 '24 13:07 sindresorhus

I think a rule could prefer withResolvers when it finds code taking resolve/reject functions out of new Promise(), like

let resolve;
const promise = new Promise(res => {
  resolve = res
})
return [promise, resolve]

fregante avatar Aug 17 '24 04:08 fregante

👍

sindresorhus avatar Aug 17 '24 11:08 sindresorhus

Accepted, with the mentioned limited scope.

sindresorhus avatar Sep 08 '24 18:09 sindresorhus

Preferred rule name? prefer-promise-with-resolvers?

omril1 avatar Dec 27 '24 20:12 omril1

I've been using the following pattern to await a dialog to close/confirm

const {promise, resolve} = Promise.withResolvers();
Object.assign(fooDialog, {show: true, resolve});
await promise;

But turns out

await new Promise((resolve) ==> Object.assign(fooDialog, {show: true, resolve}))

is simpler.

Though this matches https://github.com/sindresorhus/eslint-plugin-unicorn/issues/2403#issuecomment-2294624254

taking resolve/reject functions out of new Promise()

I guess this should not be reported.

Maybe we should also consider how the promise is used.

@fregante

fisker avatar Jan 23 '25 02:01 fisker