-
-
Notifications
You must be signed in to change notification settings - Fork 91
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
feat(no-callback-in-promise
): add timeoutsErr
option
#514
base: main
Are you sure you want to change the base?
feat(no-callback-in-promise
): add timeoutsErr
option
#514
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #514 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 25 26 +1
Lines 649 711 +62
Branches 250 277 +27
=========================================
+ Hits 649 711 +62 ☔ View full report in Codecov by Sentry. |
0e80f70
to
e0453c4
Compare
e0453c4
to
6b3dce6
Compare
I think the approach here may not be right, as we're looking for the timer functions with callbacks, not just timer functions. eg if we use the import { setImmediate } from 'timers/promises';
whatever.then((err) => setImmediate('value')) |
Ah, good catch. Do you have a suggested way of catching those? Just introspecting on the imports for known promise APIs and failing otherwise? |
The first way that comes to mind is using the But I think that it is going about things the wrong way round Maybe the check could be as simple as checking the scope of the variable and if it's got declarations? |
You mean because a declaration would itself imply a potentially Promise-based library import as opposed to the built-ins? |
…t-community#167 Also: - fix(`no-callback-in-promise`): ensure timeouts do not err (by default); fixes eslint-community#220
6b3dce6
to
cee71eb
Compare
Actually, the code is only checking for timers with callbacks. Your example currently passes with the PR. The main idea with the timeouts in this PR is to permit them unless the option I was thinking you were suggesting that the following could be more acceptable with the option set: import { setImmediate } from 'timers/promises';
whatever.then((err) => setImmediate(cb)) ...but I'm not sure that it is, as it is still passing around a callback inside of a promise. |
Ah, that makes sense! |
What is the purpose of this pull request?
What changes did you make? (Give an overview)
no-callback-in-promise
): addtimeoutsErr
option; fixes no-callback-in-promise misses callback passed as argument #167no-callback-in-promise
): ensure timeouts do not err (by default); fixes no-callback-in-promise use cases #220