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 in the no-loop-func
rule
#17528
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for docs-eslint canceled.
|
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.
Code LGTM. Can you also add a note in the rule details that explains that IIFEs are ignored while generators/async are not?
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.
LGTM. Just a suggestion clean up the docs.
I think we could simplify this a lot by taking a different approach, that is to skip IIFEs (that are not async, generators, or self-referencing) here and thus let the rule check and report their inner functions directly. eslint/lib/rules/no-loop-func.js Lines 47 to 52 in bd7a71f
For example, the current version of this rule reports error for unsafe reference to /* eslint no-loop-func: "error" */
var arr = [];
for (var i = 0; i < 5; i++) {
arr.push((
() => { // <-- error here
return () => i;
}
)());
} After this change, it would report the same error directly on the inner function: /* eslint no-loop-func: "error" */
var arr = [];
for (var i = 0; i < 5; i++) {
arr.push((
() => {
return () => i; // <-- error here
}
)());
}
The only downside is that this can change error locations (and thus existing |
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. |
Not Stale. @snitin315 is less active at the moment. Will be back soon. |
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 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. |
@mdjermanovic Once we changed error locations for |
I think it's okay in this case. For |
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. |
Makes sense, I'll update the PR 👍🏻 |
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. |
Ping @snitin315 |
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. |
5dd9f24
to
7a9f721
Compare
@mdjermanovic Thanks for the detailed review, I've resolved and added corresponding test cases to your comments. |
// We don't need to check nested functions. | ||
return null; | ||
// We need to check nested functions only in case of IIFE. | ||
if (isIIFE(node)) { |
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.
if (isIIFE(node)) { | |
if (isIIFE(parent)) { |
if (!(node.async || node.generator)) { | ||
const isFunctionExpression = node.type === "FunctionExpression"; | ||
|
||
if (isIIFE(node)) { | ||
|
||
// Check if the function is referenced elsewhere in the code | ||
const isFunctionReferenced = isFunctionExpression && node.id ? references.some(r => r.identifier.name === node.id.name) : false; | ||
|
||
// Check if there are unsafe references used within non-immediately-invoked nested functions | ||
const unsafeRefsInNonIIFE = unsafeRefs.filter(r => { | ||
let refScope = r.from.variableScope; | ||
|
||
if (node === refScope.block) { | ||
return !isIIFE(refScope.block); | ||
} | ||
|
||
while (node !== refScope.block) { | ||
if (!isIIFE(refScope.block)) { | ||
return true; | ||
} | ||
refScope = refScope.upper.variableScope; | ||
} | ||
|
||
return false; | ||
}); | ||
|
||
const unsafeRefsInNonIIFEName = unsafeRefsInNonIIFE.map(r => r.identifier.name); | ||
|
||
if (!isFunctionReferenced && unsafeRefsInNonIIFEName.length === 0) { | ||
return; | ||
} | ||
|
||
if (unsafeRefsInNonIIFEName.length > 0) { | ||
context.report({ | ||
node, | ||
messageId: "unsafeRefs", | ||
data: { varNames: `'${unsafeRefsInNonIIFEName.join("', '")}'` } | ||
}); | ||
return; | ||
} | ||
} | ||
} |
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.
There is no need to check inner functions because they'll be checked separately.
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.
Apologies for the delayed respose. Do you mean we can remove this whole logic?
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[x] 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)
Fixes #16902
Is there anything you'd like reviewers to focus on?