Rationale behind avoid-new?

[Anahkiasen]

I'm not sure I understand why this rule is in the recommended ruleset, it doesn't say why new Promise should be avoided in the docs (in case of misuse?). And I find the fact it recommends a third party package instead a bit (p)iffy.

I see the rule has been added in 2016, is it still relevant today now that promises are more widely supported?

[macklinu]

These are good questions and things we should either reconsider or document more clearly why the avoid-new rule is recommeneded. @xjamundx, do you have more insight on this?

[xjamundx]

And I find the fact it recommends a third party package instead a bit (p)iffy.

That was clever, 🤣

1. As far as what's recommended. I'm happy modifying that. I don't have strong feelings and never use it myself.

2. In my experience of using promises over the last 2 years extensively in node.js I don't think I ever needed to use new Promise and each time I did see one in a code review, they either could have used Promise.resolve() or util.promisify() or similar instead and the code would have been much tidier and more straight forward. My general thought is that new Promise may make sense in some utility libraries, but almost never in application code.

Here's an example of a few versions of the same thing:

new Promise((resolve, reject)  => {
   resolve("4")
})

// vs.
Promise.resolve("4")

new Promise((resolve, reject)  => {
  doSomethingLong("please", function(err, data) {
    if (err) return reject(err)
    resolve(data)
})

// vs.
let longThing = require('util').promisify(doSomethingLong)
longThing("please")

util.promisify and some of the other libraries also have support for special cases that don't follow the standard callback pattern such as setTimeout and setImmediate, so again there's very little reason to use new Promise

Now I know eslint-plugin-promise is used by more than node.js people, so linking to something like util.promisify may not work for everyone (and plenty of people aren't on node 8 yet), but do I make a reasonable case here?

[macklinu]

I think you both make a reasonable case. I'm also not tied to it being in the recommended ruleset. A couple PRs are welcome:

• update the avoid-new documentation to explain why this rule exists and why you might want to enable it
• update the recommended ruleset to turn off the avoid-new rule feat: turn off avoid-new in recommended configuration #119

[MrTrick]

@xjamundx your examples are good for Node, what about a browser-targeted library?

[Anahkiasen]

This was my use case as well, I've mostly bumped into this rule on client-side code which is why it was so hard for me to make sense of it.

[macklinu]

Good point about browser-based code, where the Promise API is almost entirely supported by all major browsers. Seems like overkill to include a third-party library just to prefer a promisify() function when you could use new Promise((resolve, reject) => {}).

#119 is open to remove this rule from the recommended ruleset. I aim to take care of some outstanding PRs and ship a minor release in the coming days that will include this. 👍

[sindresorhus]

Seems like overkill to include a third-party library just to prefer a promisify() function when you could use new Promise((resolve, reject) => {}).

It's about creating clean and readable code. A promisified method is much more readable than wrapping something in new Promise(). It's also very easy to make mistakes when using new Promise(), like forgetting to handle the error case correctly. Pify is only 500 bytes minified and gzipped.

However, I agree the rule is not suitable for the recommended preset. I just wanted to point out that most of the time you feel like using new Promise, there's probably a better way.

[Lonniebiz]

Here's my specific client-side function where I'm wanting to avoid this warning:

async function delay(ms)
{
	return new Promise((resolve)=>{ setTimeout(resolve, ms); });
}

Example usage:

async function main()
{
	// Wait 5 seconds:
	await delay(5000)
	console.log(`Its been 5 seconds.`)
}
main(); // says: "Its been 5 seconds." (after 5 seconds)

Without using a 3rd party library, can you think of an elegant way of not using new Promise in the above function? I can't.

Does the eslint-plugin-promise plugin provide comment directives that would allow me to override the warning for only this function?

To me, the usefulness of this rule is to avoid those circumstance when you create a new promise when you didn't need to. I like removing those, but here I needed to (I think), so I'd like to override having to see that warning each build. Please advise.