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

resolve-async should support passing callback to specific functions such as .then #87

Open
platinumazure opened this issue May 16, 2020 · 3 comments

Comments

@platinumazure
Copy link
Owner

Follow-up from #68.

The goal of this enhancement request is to support passing async callbacks to other functions, which the user expects will involve the async callback. For example, a user might pass an async callback to a promise's .then method.

Since we don't have type information available, we probably don't want to have any defaults (not even assuming .then is for promises). So I would prefer making this a rule option where users could specify an ESQuery expression or similar. Not sure what the best user experience would be, though. Open to suggestions.

@Krinkle
Copy link
Contributor

Krinkle commented Apr 5, 2021

Another one might be setTimeout. I noticed this popping up when applying the latest recommended ruleset to QUnit's own test cases:

/qunit/test/cli/fixtures/unhandled-rejection.js
  4:2  error  Async callback "done" is not called  qunit/resolve-async

https://github.com/qunitjs/qunit/blob/c26330824d9030baa5bbaf75074441e5b6751a20/test/cli/fixtures/unhandled-rejection.js#L7-L22

Granted, I could also see this being useful since it is odd use of setTimeout. If I saw this in code review anywhere outside the qunit repo or outside demo files, I would flag it as an issue. But I can also see it being annoying that "simple" demos are rejected. I think using it with Promise#then and setTimeout while perhaps not indicative of good quality code, is also very unlikely to be indicative of a mistake that deserves a lint warning. Ultimately, it does not seem helpful I think, so I'd support changing the rule to just allow these.

@XhmikosR
Copy link

Hey, guys. I was just about to report this myself.

I made a PR twbs/bootstrap#32270 to add the plugin to our v4-dev branch and I think the setTimeout is a false positive, for example:

setTimeout(done, 500)

@platinumazure
Copy link
Owner Author

This is a known limitation at present. Please use ESLint disable comments for false positives for now.

We need to design a user experience for what the rule options should look like, to allow users to allow whatever known false positives to be allowed. It would be ideal to make this as flexible as possible for most scenarios.

Ideas welcome!

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