feat(eslint-plugin): add prefer-array-methods lint rule#358
feat(eslint-plugin): add prefer-array-methods lint rule#358
Conversation
Add a new ESLint rule that flags for loops without control flow breaks (break, continue, return), encouraging the use of array methods like .forEach, .map, etc. The rule: - Flags `for` and `for...of` loops without outer-level control flow - Allows loops with break/continue/return at the outer level - Ignores control flow inside nested functions, inner loops, or switches - Shows a specialized message for loops with `await`, suggesting `Promise.all()` or `forEachAsyncSequentially` from @clipboard-health/util-ts Also adds the rule to eslint-config for all TypeScript files. GRO-1729 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a new ESLint rule "prefer-array-methods" (implementation, utilities, tests, README), registers it in the plugin and package.json, and enables the rule as an error for TypeScript files in the shared ESLint config when outside the core-utils monorepo. Changes
Sequence Diagram(s)sequenceDiagram
participant Editor
participant ESLint
participant Plugin
participant Rule
participant VisitorKeys
Editor->>ESLint: Save / Lint request
ESLint->>Plugin: Load registered rules
Plugin->>Rule: Invoke rule on ForStatement / ForOfStatement
Rule->>VisitorKeys: get child keys / inspect children
VisitorKeys-->>Rule: Return child property keys / nodes
Rule-->>ESLint: Report findings (preferArrayMethods / awaitInLoop)
ESLint-->>Editor: Present diagnostics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 75bfb58
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/eslint-plugin/src/lib/rules/prefer-array-methods/index.ts`:
- Around line 34-53: The function hasAnOuterLevelControlFlowStatement currently
skips switch statements entirely which misses return/continue inside switch
cases; update it to traverse into switches but treat only break statements as
non-outer control flow. Concretely, keep the early-return for isFunction(node)
and isForLoop(node) to avoid nested functions/loops, but when
isSwitchStatement(node) is true call forAnyChildNode with a small wrapper
predicate that returns false for isBreakStatement(child) and otherwise delegates
to hasAnOuterLevelControlFlowStatement(child) so returns/continues inside switch
cases are detected while breaks are ignored.
🧹 Nitpick comments (2)
packages/eslint-plugin/src/lib/rules/prefer-array-methods/index.ts (2)
12-25: Preferfunctiondeclarations overconstarrows for helpers.To align with the TS style guide, switch the helper predicates (and the inner
reportIfForLoopNotAllowed) tofunctiondeclarations.As per coding guidelines, Use `function` keyword for function declarations instead of `const` in TypeScript.🔧 Example refactor
-const isFunction = (node: TSESTree.Node): boolean => - node.type === AST_NODE_TYPES.ArrowFunctionExpression || - node.type === AST_NODE_TYPES.FunctionExpression || - node.type === AST_NODE_TYPES.FunctionDeclaration; +function isFunction(node: TSESTree.Node): boolean { + return ( + node.type === AST_NODE_TYPES.ArrowFunctionExpression || + node.type === AST_NODE_TYPES.FunctionExpression || + node.type === AST_NODE_TYPES.FunctionDeclaration + ); +}Also applies to: 89-109
72-115: Add explicit types forcreate(context)and its return.Typing the
contextparameter and return type makes the rule contract explicit and matches the TS guidelines.As per coding guidelines, Explicitly define parameter types and return types on functions in TypeScript.🔧 Example typing
-import { AST_NODE_TYPES, type TSESTree } from "@typescript-eslint/utils"; +import { AST_NODE_TYPES, type TSESLint, type TSESTree } from "@typescript-eslint/utils"; + +type MessageIds = "preferArrayMethods" | "awaitInLoop"; +type Options = []; @@ - create(context) { + create(context: TSESLint.RuleContext<MessageIds, Options>): TSESLint.RuleListener {
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/eslint-plugin/src/lib/rules/prefer-array-methods/index.spec.ts`:
- Around line 47-54: Add a new valid test case to the test cases array in
packages/eslint-plugin/src/lib/rules/prefer-array-methods/index.spec.ts to cover
`continue` inside a switch (similar to the existing "return inside switch"
cases); specifically add an entry named like "for...of loop with continue inside
switch statement" whose code is `for (const item of items) { switch (item.type)
{ case 'a': continue; } process(item); }` so the test suite will detect the
current bug where `continue` inside `switch` is not treated as allowing the
loop.
In `@packages/eslint-plugin/src/lib/rules/prefer-array-methods/index.ts`:
- Around line 31-45: The current hasOuterLevelBreakOrContinue treats
isSwitchStatement as a hard stop so BreakStatements are ignored (correct) but
ContinueStatements inside switches are also ignored (wrong); change the logic so
switch nodes are traversed but with BreakStatements suppressed only within the
switch. Concretely: remove the early return for isSwitchStatement in
hasOuterLevelBreakOrContinue, and add a special-case branch for nodes where
isSwitchStatement(node) is true that calls forAnyChildNode over the switch's
children while treating AST_NODE_TYPES.BreakStatement as non-exiting (return
false) but still allowing AST_NODE_TYPES.ContinueStatement to return true; keep
the existing early stops for isFunction and isForLoop. Use the existing helpers
hasOuterLevelBreakOrContinue, forAnyChildNode, isSwitchStatement, isFunction,
isForLoop and the AST_NODE_TYPES constants to locate and implement this change.
- Around line 98-102: The awaitInLoop message in the messages object embeds a
newline plus leading spaces due to the multi-line template literal (symbols:
messages, awaitInLoop, STYLE_GUIDE_URL), causing unwanted whitespace in lint
output; fix it by replacing the multi-line template literal with a single-line
template string or explicit concatenation (e.g., join the lines or use + to
combine strings) so the rendered message contains no embedded
newline/indentation while still including STYLE_GUIDE_URL and the same content;
update the messages.awaitInLoop value accordingly.
🧹 Nitpick comments (1)
packages/eslint-plugin/src/lib/rules/prefer-array-methods/index.ts (1)
12-24: Predicate helpers use arrow functions instead offunctiondeclarations.Per your coding guidelines, prefer
functionkeyword for function declarations. These three predicates (isFunction,isForLoop,isSwitchStatement) are file-scoped helpers and would be more consistent asfunctiondeclarations. This is a minor style nit given they're small predicates.
packages/eslint-plugin/src/lib/rules/prefer-array-methods/index.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/eslint-plugin/src/lib/rules/prefer-array-methods/index.ts`:
- Around line 108-110: Add an explicit return type to the function
hasABreakInControlThatWouldStopOuterForLoop — it currently lacks a return
annotation while other functions do. Update the signature of
hasABreakInControlThatWouldStopOuterForLoop to include the appropriate return
type (boolean) so it reads like the other helpers and matches the return type of
containsNodesThatWouldStopOuterForLoop.
🧹 Nitpick comments (3)
packages/eslint-plugin/src/lib/rules/prefer-array-methods/index.ts (3)
52-85: Solid refactoring of the control-flow traversal logic.The single-pass approach with narrowing
nodeTypesThatCauseOuterLoopToStopEarlyat each boundary is clean and correctly handles the nuanced semantics:
- Functions: stop all control-flow detection ✓
- Inner for loops: only propagate
return✓- Switch: filter out
breakbut propagatecontinue/return✓Nit: parameter
nodeon line 76 shadows the outer function parameter.Consider renaming to
childfor consistency with line 82.- return forAnyChildNode(node, (node: TSESTree.Node): boolean => - containsNodesThatWouldStopOuterForLoop(node, nodeTypesWithoutBreakStatement), + return forAnyChildNode(node, (child: TSESTree.Node): boolean => + containsNodesThatWouldStopOuterForLoop(child, nodeTypesWithoutBreakStatement), );
56-58: Type assertionas ControlFlowTypecan be avoided.Per coding guidelines, type assertions should be avoided unless absolutely necessary. You can use a type-safe check instead:
Suggested fix
- if (nodeTypesThatCauseOuterLoopToStopEarly.includes(node.type as ControlFlowType)) { + if ( + (nodeTypesThatCauseOuterLoopToStopEarly as readonly string[]).includes(node.type) + ) {Or alternatively, use
.some():- if (nodeTypesThatCauseOuterLoopToStopEarly.includes(node.type as ControlFlowType)) { + if (nodeTypesThatCauseOuterLoopToStopEarly.some((type) => type === node.type)) {The
.some()approach avoids all assertions entirely. As per coding guidelines, "Avoid type assertions (as,!) in TypeScript unless absolutely necessary".
9-10: Consider whether a Notion link is stable enough for lint rule messages.If the Notion page is reorganized, renamed, or access-restricted, the link in every lint violation will be broken. You might want to point to a more permanent location (e.g., a markdown file in the repo or a redirect URL you control).
| function hasABreakInControlThatWouldStopOuterForLoop(node: TSESTree.Node) { | ||
| return containsNodesThatWouldStopOuterForLoop(node, ALL_CONTROL_FLOW_TYPES); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing explicit return type annotation.
All other functions in this file declare their return types. This one is missing it.
-function hasABreakInControlThatWouldStopOuterForLoop(node: TSESTree.Node) {
+function hasABreakInControlThatWouldStopOuterForLoop(node: TSESTree.Node): boolean {As per coding guidelines, "Explicitly define parameter types and return types on functions in TypeScript".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function hasABreakInControlThatWouldStopOuterForLoop(node: TSESTree.Node) { | |
| return containsNodesThatWouldStopOuterForLoop(node, ALL_CONTROL_FLOW_TYPES); | |
| } | |
| function hasABreakInControlThatWouldStopOuterForLoop(node: TSESTree.Node): boolean { | |
| return containsNodesThatWouldStopOuterForLoop(node, ALL_CONTROL_FLOW_TYPES); | |
| } |
🤖 Prompt for AI Agents
In `@packages/eslint-plugin/src/lib/rules/prefer-array-methods/index.ts` around
lines 108 - 110, Add an explicit return type to the function
hasABreakInControlThatWouldStopOuterForLoop — it currently lacks a return
annotation while other functions do. Update the signature of
hasABreakInControlThatWouldStopOuterForLoop to include the appropriate return
type (boolean) so it reads like the other helpers and matches the return type of
containsNodesThatWouldStopOuterForLoop.
1 participant
Summary
This adds a new ESLint rule,
prefer-array-methods, that flagsfor loopswithout control flow breaks. This is to adhere with our typescript best practice to Favor array methods over for loops (when not breaking).Also adds this rule to
eslint-configfor all TypeScript files.Motivation
Array methods are more declarative and functional, and provide readability, clarity of intent, and method chaining.
However, if the loop can break early (e.g. for optimizing code), then it is preferred to use the for loop. This is because functional loops like
mapandforEachcannot be exited early. (someandeverycan exit early; however, using them to do so just for early exiting could lead to a lack of readability).Examples
Specialized Error Message For Await
In addition, this rule shows a specialized error message if the for loop has
awaitin it. This is becauseawaitdoesn't work in array methods like you think it would. For instance:does not wait for
process(1)to finish before moving on toprocess(2).However; this is different than the behavior of
which does not begin invoking
process(2)untilprocess(1)finishes.In order to make sure engineers don't accidentally change behavior, we make this more verbose error message.
This specialized error message refers to potentially using forEachAsyncSequentially.
Later work will inventory the usages of that method to see if we can make calls in parallel instead
Related
forEachAsyncSequentiallyfrom @clipboard-health/util-ts