From 5364d257d54b95d40fb560b5aacbafe9e0c81fa2 Mon Sep 17 00:00:00 2001 From: Javier Gonzalez Date: Fri, 26 Sep 2025 11:43:12 +0200 Subject: [PATCH 1/3] fix: allow eslint react hooks rule to apply to any wrapped components --- .../__tests__/ESLintRulesOfHooks-test.js | 52 ++++++++ .../src/rules/RulesOfHooks.ts | 121 +++++++++++++----- 2 files changed, 140 insertions(+), 33 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js index 2f3e14c5f95e4..f4412c1f659fa 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js @@ -1353,6 +1353,58 @@ const allTests = { `, errors: [tryCatchUseError('use')], }, + { + code: normalizeIndent` + // Invalid because rules-of-hooks must also apply to components wrapped in wrapper functions. + // This is a case where it wraps a properly named function. + // This *must* be invalid. + const ComponentWithConditionalHook = anyWrapper(function ComponentWithConditionalHook() { + if (cond) { + useConditionalHook(); + } + }) + `, + errors: [conditionalError('useConditionalHook')], + }, + { + code: normalizeIndent` + // Invalid because rules-of-hooks must also apply to components wrapped in wrapper functions. + // This is a case where it wraps an anonymous function. + // This *must* be invalid. + const ComponentWithConditionalHook = anyWrapper(function ComponentWithConditionalHook() { + if (cond) { + useConditionalHook(); + } + }) + `, + errors: [conditionalError('useConditionalHook')], + }, + { + code: normalizeIndent` + // Invalid because rules-of-hooks must also apply to components wrapped in wrapper functions. + // This is a case where it wraps an arrow function. + // This *must* be invalid. + const ComponentWithConditionalHook = anyWrapper(() => { + if (cond) { + useConditionalHook(); + } + }) + `, + errors: [conditionalError('useConditionalHook')], + }, + { + code: normalizeIndent` + // Invalid because rules-of-hooks must also apply to components wrapped in many wrapper functions. + // This is a case where it double wraps an arrow function. + // This *must* be invalid. + const ComponentWithConditionalHook = anyWrapper1(anyWrapper2(() => { + if (cond) { + useConditionalHook(); + } + })) + `, + errors: [conditionalError('useConditionalHook')], + }, ], }; diff --git a/packages/eslint-plugin-react-hooks/src/rules/RulesOfHooks.ts b/packages/eslint-plugin-react-hooks/src/rules/RulesOfHooks.ts index 4c7618d8e084c..0921a1ce80df3 100644 --- a/packages/eslint-plugin-react-hooks/src/rules/RulesOfHooks.ts +++ b/packages/eslint-plugin-react-hooks/src/rules/RulesOfHooks.ts @@ -68,32 +68,6 @@ function isReactFunction(node: Node, functionName: string): boolean { ); } -/** - * Checks if the node is a callback argument of forwardRef. This render function - * should follow the rules of hooks. - */ -function isForwardRefCallback(node: Node): boolean { - return !!( - node.parent && - 'callee' in node.parent && - node.parent.callee && - isReactFunction(node.parent.callee, 'forwardRef') - ); -} - -/** - * Checks if the node is a callback argument of React.memo. This anonymous - * functional component should follow the rules of hooks. - */ -function isMemoCallback(node: Node): boolean { - return !!( - node.parent && - 'callee' in node.parent && - node.parent.callee && - isReactFunction(node.parent.callee, 'memo') - ); -} - function isInsideComponentOrHook(node: Node | undefined): boolean { while (node) { const functionName = getFunctionName(node); @@ -102,8 +76,12 @@ function isInsideComponentOrHook(node: Node | undefined): boolean { return true; } } - if (isForwardRefCallback(node) || isMemoCallback(node)) { - return true; + const functionNameSkippingCallExpressions = + getFunctionNameSkippingCallExpressions(node); + if (functionNameSkippingCallExpressions) { + if (isComponentName(functionNameSkippingCallExpressions)) { + return true; + } } node = node.parent; } @@ -148,7 +126,12 @@ function getNodeWithoutReactNamespace( } function isEffectIdentifier(node: Node): boolean { - return node.type === 'Identifier' && (node.name === 'useEffect' || node.name === 'useLayoutEffect' || node.name === 'useInsertionEffect'); + return ( + node.type === 'Identifier' && + (node.name === 'useEffect' || + node.name === 'useLayoutEffect' || + node.name === 'useInsertionEffect') + ); } function isUseEffectEventIdentifier(node: Node): boolean { if (__EXPERIMENTAL__) { @@ -485,10 +468,27 @@ const rule = { // function component or we are in a hook function. const isSomewhereInsideComponentOrHook = isInsideComponentOrHook(codePathNode); - const isDirectlyInsideComponentOrHook = codePathFunctionName - ? isComponentName(codePathFunctionName) || - isHook(codePathFunctionName) - : isForwardRefCallback(codePathNode) || isMemoCallback(codePathNode); + + const isDirectlyInsideComponentOrHook = (() => { + if ( + codePathFunctionName && + (isComponentName(codePathFunctionName) || + isHook(codePathFunctionName)) + ) { + return true; + } + + const codePathFunctionNameSkippingCallExpressions = + getFunctionNameSkippingCallExpressions(codePathNode); + if ( + codePathFunctionNameSkippingCallExpressions && + isComponentName(codePathFunctionNameSkippingCallExpressions) + ) { + return true; + } + + return false; + })(); // Compute the earliest finalizer level using information from the // cache. We expect all reachable final segments to have a cache entry @@ -853,6 +853,61 @@ function getFunctionName(node: Node) { } } +/** + * Gets the static name of a function that is passed as an argument to one or more + * wrapper function calls. This function traverses up the AST through multiple levels + * of CallExpression nodes to find the ultimate variable assignment. + * + * This is used to identify React components that are wrapped in higher-order components + * or other wrapper functions like React.memo, React.forwardRef, or custom wrappers. + * + * The function works by: + * 1. Starting with the given function node + * 2. Traversing up through parent CallExpression nodes while the current node + * is an argument to those calls + * 3. Stopping when it reaches a VariableDeclarator that assigns the final + * CallExpression result to a variable + * 4. Returning the variable identifier if found + * + * This enables the rules-of-hooks linter to properly identify components even + * when they are deeply nested in wrapper function calls, allowing it to apply + * React Hook rules correctly. + */ + +function getFunctionNameSkippingCallExpressions(node: Node) { + if ( + node.type === 'FunctionExpression' || + node.type === 'ArrowFunctionExpression' + ) { + let current: Node = node; + let parent = node.parent; + + // Skip through multiple chained CallExpressions + while ( + parent?.type === 'CallExpression' && + parent.arguments.includes(current) + ) { + current = parent; + parent = parent.parent; + } + + if (parent?.type === 'VariableDeclarator' && parent.init === current) { + // const ComponentWithConditionalHook = memo(() => {}); + // const ComponentWithConditionalHook = forwardRef(() => {}); + // const ComponentWithConditionalHook = anyWrapper(() => {}); + // const ComponentWithConditionalHook = anyWrapper1(anyWrapper2(() => {})); + // + // This handles the case where a function is passed as an argument to + // one or more wrapper functions that are assigned to a component-named variable. + return parent.id; + } else { + return undefined; + } + } else { + return undefined; + } +} + /** * Convenience function for peeking the last item in a stack. */ From 1720665375df768ba4396918f0caaa32c6679725 Mon Sep 17 00:00:00 2001 From: Javier Gonzalez Date: Fri, 26 Sep 2025 11:51:35 +0200 Subject: [PATCH 2/3] fix: fix anonymous function test --- .../__tests__/ESLintRulesOfHooks-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js index f4412c1f659fa..13d307448ec99 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js @@ -1371,7 +1371,7 @@ const allTests = { // Invalid because rules-of-hooks must also apply to components wrapped in wrapper functions. // This is a case where it wraps an anonymous function. // This *must* be invalid. - const ComponentWithConditionalHook = anyWrapper(function ComponentWithConditionalHook() { + const ComponentWithConditionalHook = anyWrapper(function() { if (cond) { useConditionalHook(); } From 08f0f0e83251f8da16bbace243c31e0b7ae2e185 Mon Sep 17 00:00:00 2001 From: Javier Gonzalez Date: Fri, 26 Sep 2025 12:00:13 +0200 Subject: [PATCH 3/3] refactor: update variable names in comments for clarity in RulesOfHooks.ts --- .../eslint-plugin-react-hooks/src/rules/RulesOfHooks.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/src/rules/RulesOfHooks.ts b/packages/eslint-plugin-react-hooks/src/rules/RulesOfHooks.ts index 0921a1ce80df3..18a80c594f87c 100644 --- a/packages/eslint-plugin-react-hooks/src/rules/RulesOfHooks.ts +++ b/packages/eslint-plugin-react-hooks/src/rules/RulesOfHooks.ts @@ -892,10 +892,10 @@ function getFunctionNameSkippingCallExpressions(node: Node) { } if (parent?.type === 'VariableDeclarator' && parent.init === current) { - // const ComponentWithConditionalHook = memo(() => {}); - // const ComponentWithConditionalHook = forwardRef(() => {}); - // const ComponentWithConditionalHook = anyWrapper(() => {}); - // const ComponentWithConditionalHook = anyWrapper1(anyWrapper2(() => {})); + // const ComponentName = memo(() => {}); + // const ComponentName = forwardRef(() => {}); + // const ComponentName = anyWrapper(() => {}); + // const ComponentName = anyWrapper1(anyWrapper2(() => {})); // // This handles the case where a function is passed as an argument to // one or more wrapper functions that are assigned to a component-named variable.