Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
BoresXP opened this issue Oct 21, 2019 · 3 comments
Open

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

BoresXP opened this issue Oct 21, 2019 · 3 comments

Comments

@BoresXP
Copy link

BoresXP commented Oct 21, 2019

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
Copy link
Contributor

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
Copy link
Author

BoresXP commented Oct 23, 2019

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
Copy link
Member

brettz9 commented Jul 20, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants