promise/prefer-await-to-callbacks warns on library methods

[BoresXP]

Description

Rule promise/prefer-await-to-callbacks incorrectly warns on library methods. It should check only code in project.

Steps to Reproduce

1. Project with electron written in TypeScript.
2. Use cookies
3. Use cookies.set method that accepts callback as last argument.

Code:

	const window = new BrowserWindow()
	const { cookies } = window.webContents.session
	cookies.on('changed', (_event, cookie, _cause, _removed) => {
		const oneDay = 24 * 60 * 60
		cookies.set(
			{
				url,
				name: cookie.name,
				value: cookie.value,
				domain: cookie.domain,
				path: cookie.path,
				secure: cookie.secure,
				httpOnly: cookie.httpOnly,
				expirationDate: Math.floor(new Date().getTime() / 1000) + oneDay,
			},
			err => {
				if (err) {
					log.error('Error trying to persist cookie', err, cookie)
				}
			},
		)
	}
})

Expected behavior: All should be fine because method comes from library (typings)

Actual behavior: err => {} is marked as error because of callback argument name (WHAT?!)

Versions

• Node version: v10.14.1
• ESLint version: 6.5.1
• eslint-plugin-promise version: 4.2.1

Additional Information

Project in TypeScript language. Just to let you know.

[macklinu]

This plugin doesn't check TypeScript types and was built before TypeScript and ESLint had more integrated tooling with each other. Right now, rules are checking names of arguments and warning, resulting in some false positives for sure.

I'm not sure the lengths we could go to in this plugin to support TypeScript type checking 🤔 definitely worth some exploration, but not something I can personally contribute time to right now. Would love some more input on this from you or other ESLint plugin contributors who have been able to support TypeScript language features in an ESLint plugin without forcing JavaScript-only codebases to install TypeScript tooling.

We also need some better docs for how/when to use the prefer-await-to-callbacks rule (#118). I think one way to promisify your usage of 3rd party library would allow you to use cookies.set() as a Promise, thus preventing the ESLint error you reported.

// https://nodejs.org/api/util.html#util_util_promisify_original
// available in Node 8 or later
import {promisify} from 'util'
// could also use the 3rd party module, pify
// https://www.npmjs.com/package/pify

const {cookies} = window.webContents.session
const setCookie = promisify(cookies.set)

cookies.on('changed', async (_, cookie) => {
  const updateCookie = cookie => ({ ...cookie, /* whatever you want to update */ })
  try {
    const newCookie = updateCookie(cookie)
    await setCookie(newCookie)
  } catch (err) {
    handleError(err)
  }
})

Alternatively, you could disable the rule for this case with an // eslint-disable-next-line promise/prefer-await-to-callbacks comment (more info about this in the ESLint docs).

I know the issue you reported isn't the ideal developer experience right now, but it's a tricky problem trying to detect usage of 3rd party libraries and Promises in an ESLint plugin, which by default, has no understanding of TypeScript types. I appreciate your feedback and let me know if any of these things help for now or if you have other questions/ideas.

[BoresXP]

Thanks for replying so fast.
It looks like rule should be fixed this way: if method, accepting callback, has it's implementation in node_modules, rule violation should not be reported. This way we will avoid warnings for third party methods we cannot fix.

[brettz9]

Thanks for replying so fast. It looks like rule should be fixed this way: if method, accepting callback, has it's implementation in node_modules, rule violation should not be reported. This way we will avoid warnings for third party methods we cannot fix.

Some projects might want to get rid of dependencies which don't offer Promises though. I think it may be reasonable as an optional enhancement, however.