diff --git a/packages/core/src/component/component-collector-legacy.ts b/packages/core/src/component/component-collector-legacy.ts index 3e0fc5d41f..b57a79d1ef 100644 --- a/packages/core/src/component/component-collector-legacy.ts +++ b/packages/core/src/component/component-collector-legacy.ts @@ -77,7 +77,7 @@ export function isThisSetState(node: TSESTree.CallExpression) { const { callee } = node; return ( callee.type === T.MemberExpression - && AST.isThisExpression(callee.object) + && AST.isThisExpressionLoose(callee.object) && callee.property.type === T.Identifier && callee.property.name === "setState" ); @@ -91,6 +91,6 @@ export function isThisSetState(node: TSESTree.CallExpression) { export function isAssignmentToThisState(node: TSESTree.AssignmentExpression) { const { left } = node; return left.type === T.MemberExpression - && AST.isThisExpression(left.object) + && AST.isThisExpressionLoose(left.object) && AST.getPropertyName(left.property) === "state"; } diff --git a/packages/plugins/eslint-plugin-react-x/src/rules/no-access-state-in-setstate.ts b/packages/plugins/eslint-plugin-react-x/src/rules/no-access-state-in-setstate.ts index 027bb9fb99..19328e70b2 100644 --- a/packages/plugins/eslint-plugin-react-x/src/rules/no-access-state-in-setstate.ts +++ b/packages/plugins/eslint-plugin-react-x/src/rules/no-access-state-in-setstate.ts @@ -93,7 +93,7 @@ export function create(context: RuleContext): RuleListener { // Main logic for detecting `this.state` access MemberExpression(node) { // Check for `this` expressions - if (!AST.isThisExpression(node.object)) { + if (!AST.isThisExpressionLoose(node.object)) { return; } // Ensure we are inside a React class component @@ -151,7 +151,7 @@ export function create(context: RuleContext): RuleListener { return; } // Check for destructuring from `this` - if (node.init == null || !AST.isThisExpression(node.init) || node.id.type !== T.ObjectPattern) { + if (node.init == null || !AST.isThisExpressionLoose(node.init) || node.id.type !== T.ObjectPattern) { return; } // Check if `state` is one of the destructured properties diff --git a/packages/plugins/eslint-plugin-react-x/src/rules/no-array-index-key.ts b/packages/plugins/eslint-plugin-react-x/src/rules/no-array-index-key.ts index 24f9a05ba8..bcdec5426d 100644 --- a/packages/plugins/eslint-plugin-react-x/src/rules/no-array-index-key.ts +++ b/packages/plugins/eslint-plugin-react-x/src/rules/no-array-index-key.ts @@ -19,6 +19,25 @@ export type MessageID = CamelCase; const REACT_CHILDREN_METHOD = ["forEach", "map"] as const; +const arrayIndexParamPosition = new Map([ + ["every", 1], + ["filter", 1], + ["find", 1], + ["findIndex", 1], + ["findLast", 1], + ["findLastIndex", 1], + ["flatMap", 1], + ["forEach", 1], + ["map", 1], + ["reduce", 2], + ["reduceRight", 2], + ["some", 1], +]); + +export function getArrayIndexParamPosition(methodName: string) { + return arrayIndexParamPosition.get(methodName) ?? -1; +} + // Checks if a method name is 'forEach' or 'map' function isReactChildrenMethod(name: string): name is typeof REACT_CHILDREN_METHOD[number] { return REACT_CHILDREN_METHOD.includes(name as never); @@ -56,7 +75,7 @@ function getMapIndexParamName(context: RuleContext, node: TSESTree.CallExpressio } const { name } = callee.property; // Determines the position of the index parameter for array methods like 'map', 'forEach', etc - const indexPosition = AST.getArrayMethodCallbackIndexParamPosition(name); + const indexPosition = getArrayIndexParamPosition(name); if (indexPosition === -1) { return unit; } diff --git a/packages/plugins/eslint-plugin-react-x/src/rules/no-unnecessary-key.spec.ts b/packages/plugins/eslint-plugin-react-x/src/rules/no-unnecessary-key.spec.ts index ccc1d7f5e5..e6c9ac40cc 100644 --- a/packages/plugins/eslint-plugin-react-x/src/rules/no-unnecessary-key.spec.ts +++ b/packages/plugins/eslint-plugin-react-x/src/rules/no-unnecessary-key.spec.ts @@ -30,7 +30,7 @@ ruleTester.run(RULE_NAME, rule, { `, errors: [{ messageId: "noUnnecessaryKey" }, { messageId: "noUnnecessaryKey" }], }, - // Invalid: unnecessary key on a child element in a map + // Invalid: unnecessary key on a child element in a map { code: tsx` things.map(thing =>

) @@ -40,7 +40,7 @@ ruleTester.run(RULE_NAME, rule, { // Invalid: redundant key on a direct child when the parent Fragment already has the key { code: tsx` - things.map(thing =>
{thing.name}
) + things.map(thing =>
{thing.name}
) `, errors: [{ messageId: "noUnnecessaryKey" }], }, @@ -73,13 +73,140 @@ ruleTester.run(RULE_NAME, rule, { `, errors: [{ messageId: "noUnnecessaryKey" }], }, - // Invalid: unnecessary key with a static value + // Invalid: unnecessary key with a static value { code: tsx` `, errors: [{ messageId: "noUnnecessaryKey" }], }, + // Invalid: static key in a simple function component + { + code: tsx` + function SimpleComponent() { + return
; + } + `, + errors: [{ messageId: "noUnnecessaryKey" }], + }, + // Invalid: static key in arrow function component + { + code: tsx` + const ArrowComponent = () => ; + `, + errors: [{ messageId: "noUnnecessaryKey" }], + }, + // Invalid: deeply nested unnecessary keys + { + code: tsx` + things.map(thing => ( +
+
+
+

Content

+
+
+
+ )) + `, + errors: [{ messageId: "noUnnecessaryKey" }], + }, + // Invalid: key on self-closing child element + { + code: tsx` + things.map(thing =>
) + `, + errors: [{ messageId: "noUnnecessaryKey" }], + }, + // Invalid: key with template literal value + { + code: tsx` +
+ `, + errors: [{ messageId: "noUnnecessaryKey" }], + }, + // Invalid: key with number literal + { + code: tsx` +
+ `, + errors: [{ messageId: "noUnnecessaryKey" }], + }, + // Invalid: unnecessary key in class component render + { + code: tsx` + class MyComponent extends React.Component { + render() { + return
; + } + } + `, + errors: [{ messageId: "noUnnecessaryKey" }], + }, + // Invalid: key on child with sibling elements (not in list context) + { + code: tsx` + function Component() { + return ( +
+ + +
+ ); + } + `, + errors: [{ messageId: "noUnnecessaryKey" }, { messageId: "noUnnecessaryKey" }], + }, + // Invalid: unnecessary key in flatMap callback + { + code: tsx` + things.flatMap(thing =>
) + `, + errors: [{ messageId: "noUnnecessaryKey" }], + }, + // Invalid: key on wrapper and child in reduce + // { + // code: tsx` + // items.reduce((acc, item) => [... acc,

], []) + // `, + // errors: [{ messageId: "noUnnecessaryKey" }], + // }, + // Invalid: unnecessary keys in forEach (not returning elements, but still detected) + { + code: tsx` + function Component() { + return
; + } + `, + errors: [{ messageId: "noUnnecessaryKey" }, { messageId: "noUnnecessaryKey" }], + }, + // Invalid: key in immediately invoked function expression + { + code: tsx` + const element = (() =>
)(); + `, + errors: [{ messageId: "noUnnecessaryKey" }], + }, + // Invalid: multiple levels of unnecessary keys + { + code: tsx` + things.map(thing => ( +
+
    +
  • Content
  • +
+
+ )) + `, + errors: [{ messageId: "noUnnecessaryKey" }, { messageId: "noUnnecessaryKey" }], + }, + // Invalid: key on element inside JSX expression container + { + code: tsx` + things.map(thing =>
{true && }
) + `, + errors: [{ messageId: "noUnnecessaryKey" }], + }, ], valid: [ tsx` @@ -92,7 +219,7 @@ ruleTester.run(RULE_NAME, rule, { tsx` things.map(thing =>
) `, - // Valid: key on the top-level React.Fragment in a map + // Valid: key on the top-level React. Fragment in a map tsx` things.map(thing =>
) `, @@ -111,10 +238,10 @@ ruleTester.run(RULE_NAME, rule, { things.map(thing =>

Hello

) `, tsx` - things.map(thing =>

); + things.map(thing =>

); `, tsx` - things.map(thing =>

); + things.map(thing =>

); `, tsx` function IResetChildrenOnWritingDirectionChange1({ lang = "en", children }: { lang?: "en" | "fr" | "ar"; children: React.ReactNode }) { @@ -133,5 +260,151 @@ ruleTester.run(RULE_NAME, rule, { return
{children}
; } `, + // FIXME: This case should be valid, but currently reports a false positive + // tsx` + // function IResetChildrenOnWritingDirectionChange2({ lang = "en", children }: { lang?: "en" | "fr" | "ar"; children: React.ReactNode }) { + // if (lang === "ar") return
{children}
; + // return
{children}
; + // } + // `, + // Valid: key in ternary expression (conditional rendering) + tsx` + function Component({ isActive }) { + return isActive ?
:
; + } + `, + // Valid: key in logical AND expression + tsx` + function Component({ show }) { + return show &&
; + } + `, + // Valid: key in logical OR expression + tsx` + function Component({ element }) { + return element ||
; + } + `, + // Valid: key in nullish coalescing expression + tsx` + function Component({ element }) { + return element ??
; + } + `, + // Valid: key on top-level element in flatMap + tsx` + things.flatMap(thing =>
) + `, + // Valid: key on top-level element in filter().map() chain + tsx` + things.filter(Boolean).map(thing =>
) + `, + // Valid: key on element returned from switch statement + tsx` + function Component({ type }) { + switch (type) { + case 'a': return
; + case 'b': return
; + default: return
; + } + } + `, + // Valid: key on element in if-else branches + tsx` + function Component({ condition }) { + if (condition) { + return
; + } else { + return
; + } + } + `, + // Valid: key on nested map (inner map needs its own key) + tsx` + outers.map(outer => ( +
+ {outer.inners.map(inner => )} +
+ )) + `, + // Valid: key on element stored in variable then used in array + tsx` + const item1 =
; + const item2 =
; + const items = [item1, item2]; + `, + // Valid: Fragment shorthand in map with keyed children + tsx` + things.map(thing => ) + `, + // Valid: key on top-level element in async map-like pattern + tsx` + Promise.all(things.map(async thing =>
)) + `, + // Valid: key on object property value in array context + tsx` + const config = { + items: [
,
] + }; + `, + // Valid: key on element in reduce accumulator + // tsx` + // items.reduce((acc, item) => [...acc,
], []) + // `, + // Valid: key with spread attribute (should be skipped) + tsx` + things.map(thing =>
) + `, + // Valid: key in early return pattern + tsx` + function Component({ items }) { + if (! items. length) return
No items
; + return items.map(item =>
{item.name}
); + } + `, + // Valid: key on cloned element pattern (common in HOCs) + tsx` + children.map((child, index) => React.cloneElement(child, { key: index })) + `, + // Valid: multiple conditional keys at same level + tsx` + function TabPanel({ activeTab }) { + return ( +
+ {activeTab === 'home' && } + {activeTab === 'settings' && } + {activeTab === 'profile' && } +
+ ); + } + `, + // Valid: key in optional chaining context + tsx` + items?.map(item =>
) + `, + // Valid: key on element in callback passed to custom component + tsx` + } /> + `, + // Valid: key on element inside Object.entries map + tsx` + Object.entries(obj).map(([key, value]) =>
{value}
) + `, + // Valid: key on element inside Object.keys map + tsx` + Object.keys(obj).map(key =>
{obj[key]}
) + `, + // Valid: key on element inside Object.values map with index + tsx` + Object.values(obj).map((value, index) =>
{value}
) + `, + // Valid: key on element inside Array.from map + tsx` + Array.from({ length: 5 }, (_, i) =>
) + `, + // Valid: key on element inside [... array]. map + tsx` + [...items].map(item =>
) + `, ], }); diff --git a/packages/plugins/eslint-plugin-react-x/src/rules/no-unnecessary-key.ts b/packages/plugins/eslint-plugin-react-x/src/rules/no-unnecessary-key.ts index fdbb3d3cd5..484e46434b 100644 --- a/packages/plugins/eslint-plugin-react-x/src/rules/no-unnecessary-key.ts +++ b/packages/plugins/eslint-plugin-react-x/src/rules/no-unnecessary-key.ts @@ -55,7 +55,7 @@ export function create(context: RuleContext): RuleListener { // If there is a spread attribute, it's not safe to report an unnecessary key if (jsxElement.openingElement.attributes.some((attr) => attr.type === T.JSXSpreadAttribute)) return; // Find the parent `.map()` callback function, if it exists - const mapCallback = AST.findParentNode(jsxElement, isMapCallback); + const mapCallback = AST.findParentNode(jsxElement, isArrayMethodCallback); // Check static keys on elements that are not in a map context if (mapCallback == null || AST.findParentNode(jsxElement, AST.isFunction) !== mapCallback) { // Check if the keyed element is inside a condition expression, control flow statement, or has an enclosing assignment target @@ -104,8 +104,8 @@ export function create(context: RuleContext): RuleListener { * @param node The node to check * @returns `true` if the node is a map callback, `false` otherwise */ -function isMapCallback(node: TSESTree.Node) { - if (node.parent == null) return false; - if (!AST.isArrayMapCall(node.parent)) return false; +function isArrayMethodCallback(node: TSESTree.Node) { + if (node.parent?.type !== T.CallExpression) return false; + if (!AST.isArrayMapCall(node.parent) || !AST.isArrayFromCall(node.parent)) return false; return AST.isOneOf([T.ArrowFunctionExpression, T.FunctionExpression])(AST.getUnderlyingExpression(node)); } diff --git a/packages/plugins/eslint-plugin-react-x/src/rules/no-unused-class-component-members.ts b/packages/plugins/eslint-plugin-react-x/src/rules/no-unused-class-component-members.ts index dee030fe89..06c5fb9580 100644 --- a/packages/plugins/eslint-plugin-react-x/src/rules/no-unused-class-component-members.ts +++ b/packages/plugins/eslint-plugin-react-x/src/rules/no-unused-class-component-members.ts @@ -164,7 +164,7 @@ export function create(context: RuleContext): RuleListener { return; } // Check for expressions like `this.property` - if (!AST.isThisExpression(node.object) || !isKeyLiteral(node, node.property)) { + if (!AST.isThisExpressionLoose(node.object) || !isKeyLiteral(node, node.property)) { return; } // Detect assignments like `this.property = xxx` as definitions @@ -193,7 +193,7 @@ export function create(context: RuleContext): RuleListener { return; } // Detect destructuring from `this`, e.g., `const { foo, bar } = this;` - if (node.init != null && AST.isThisExpression(node.init) && node.id.type === T.ObjectPattern) { + if (node.init != null && AST.isThisExpressionLoose(node.init) && node.id.type === T.ObjectPattern) { for (const prop of node.id.properties) { if (prop.type === T.Property && isKeyLiteral(prop, prop.key)) { const keyName = AST.getPropertyName(prop.key); diff --git a/packages/plugins/eslint-plugin-react-x/src/rules/no-unused-state.ts b/packages/plugins/eslint-plugin-react-x/src/rules/no-unused-state.ts index 6a6c607f22..4f4214bacb 100644 --- a/packages/plugins/eslint-plugin-react-x/src/rules/no-unused-state.ts +++ b/packages/plugins/eslint-plugin-react-x/src/rules/no-unused-state.ts @@ -145,7 +145,7 @@ export function create(context: RuleContext): RuleListener { "ClassExpression:exit": classExit, MemberExpression(node) { // Detect state usage (`this.state`) - if (!AST.isThisExpression(node.object)) { + if (!AST.isThisExpressionLoose(node.object)) { return; } if (AST.getPropertyName(node.property) !== "state") { @@ -194,7 +194,7 @@ export function create(context: RuleContext): RuleListener { return; } // Detect state usage via destructuring (`const { state } = this`) - if (node.init == null || !AST.isThisExpression(node.init) || node.id.type !== T.ObjectPattern) { + if (node.init == null || !AST.isThisExpressionLoose(node.init) || node.id.type !== T.ObjectPattern) { return; } const hasState = node.id.properties.some((prop) => { diff --git a/packages/utilities/ast/src/array-index.ts b/packages/utilities/ast/src/array-index.ts deleted file mode 100644 index 2d48668ebb..0000000000 --- a/packages/utilities/ast/src/array-index.ts +++ /dev/null @@ -1,18 +0,0 @@ -const indexParamPosition = new Map([ - ["every", 1], - ["filter", 1], - ["find", 1], - ["findIndex", 1], - ["findLast", 1], - ["findLastIndex", 1], - ["flatMap", 1], - ["forEach", 1], - ["map", 1], - ["reduce", 2], - ["reduceRight", 2], - ["some", 1], -]); - -export function getArrayMethodCallbackIndexParamPosition(methodName: string) { - return indexParamPosition.get(methodName) ?? -1; -} diff --git a/packages/utilities/ast/src/expression-is.ts b/packages/utilities/ast/src/expression-is.ts index ae0fff5904..f1990269ba 100644 --- a/packages/utilities/ast/src/expression-is.ts +++ b/packages/utilities/ast/src/expression-is.ts @@ -10,6 +10,6 @@ import { getUnderlyingExpression } from "./expression-base"; * @param node The expression node to check * @returns true if the expression is a ThisExpression, false otherwise */ -export function isThisExpression(node: TSESTree.Expression) { +export function isThisExpressionLoose(node: TSESTree.Expression) { return getUnderlyingExpression(node).type === T.ThisExpression; } diff --git a/packages/utilities/ast/src/index.ts b/packages/utilities/ast/src/index.ts index 2f3bd22820..9b1f414c2b 100644 --- a/packages/utilities/ast/src/index.ts +++ b/packages/utilities/ast/src/index.ts @@ -1,4 +1,3 @@ -export * from "./array-index"; export * from "./array-method"; export * from "./class-id"; export * from "./equal";