-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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: ignore IIFE's for no-loop-func rule #17426
Conversation
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -185,6 +199,10 @@ module.exports = { | |||
return; | |||
} | |||
|
|||
if (!node.async && !node.generator && isIIFE(node.parent)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a false negative:
/* eslint no-loop-func: "error" */
var arr = [];
for (var i = 0; i < 5; i++) {
arr.push((f => f)(() => i)); // false negative, this is not a call of `() => i`
}
console.log(arr.map(f => f())); // [ 5, 5, 5, 5, 5 ]
Parent of () => i
is a function call, but not a call of that function (() => i
is an argument, not callee).
Also, references in nested functions should still be considered unsafe, unless perhaps they're also immediately executed. /* eslint no-loop-func: "error" */
var arr = [];
for (var i = 0; i < 5; i++) {
arr.push((() => {
return () => i; // false negative
})());
}
console.log(arr.map(f => f())); // [ 5, 5, 5, 5, 5 ] |
Also, we should check if the function is referenced (#16902 (comment)). Another example: /* eslint no-loop-func: "error" */
var arr = [];
for (var i = 0; i < 5; i++) {
(function fun () {
if (arr.includes(fun)) return i; // false negative
else arr.push(fun);
})();
}
console.log(arr.map(f => f())); // [ 5, 5, 5, 5, 5 ] |
Makes sense, probably we can maintain a functionStack to keep track of currently running functions and use it to find if an IIFE is being referenced inside it or not. |
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
Hi @Gautam-Arora24, are you still working on this? |
Hey @Rec0iL99! Apologies, not getting enough time to work on the PR. |
@Gautam-Arora24 No worries, I'll finish the PR. |
Thanks for the PR, closing in favor of #17528 |
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
The change proposes to ignore IIFE's inside the loop's block. However, if the function inside the loop is an async or a generator function, the rule still continues to report error.
Fixes: #16902
Is there anything you'd like reviewers to focus on?