From 471a34cc8dd600a1a173c707ae211ec8a9def731 Mon Sep 17 00:00:00 2001 From: David Garner Date: Sat, 2 Nov 2019 10:43:50 +0000 Subject: [PATCH 1/5] [eslint] Check forwardRef callbacks (#17220) --- .../__tests__/ESLintRulesOfHooks-test.js | 56 +++++++++++++++++++ .../src/RulesOfHooks.js | 30 +++++++++- 2 files changed, 84 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js index 3314439cd35d5..be203be236cc0 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js @@ -141,6 +141,24 @@ const tests = { return useHook1(useHook2()); } `, + ` + // Valid because hooks can be used in forwardRef. + const FancyButton = React.forwardRef(() => { + return useHook(); + }); + `, + ` + // Valid because hooks can be used in forwardRef. + const FancyButton = React.forwardRef(function () { + return useHook(); + }); + `, + ` + // Valid because hooks can be used in forwardRef. + const FancyButton = forwardRef(function () { + return useHook(); + }); + `, ` // Valid because classes can call functions. // We don't consider these to be hooks. @@ -437,6 +455,18 @@ const tests = { `, errors: [genericError('useHookInsideCallback')], }, + { + code: ` + // Invalid because it's a common misunderstanding. + // We *could* make it valid but the runtime error could be confusing. + const ComponentWithHookInsideCallback = React.forwardRef(() => { + useEffect(() => { + useHookInsideCallback(); + }); + }); + `, + errors: [genericError('useHookInsideCallback')], + }, { code: ` // Invalid because it's a common misunderstanding. @@ -695,6 +725,32 @@ const tests = { // conditionalError('useState'), ], }, + { + code: ` + // Invalid because it's dangerous and might not warn otherwise. + // This *must* be invalid. + const FancyButton = React.forwardRef((props, ref) => { + if (props.fancy) { + useCustomHook(); + } + return ; + }); + `, + errors: [conditionalError('useCustomHook')], + }, + { + code: ` + // Invalid because it's dangerous and might not warn otherwise. + // This *must* be invalid. + const FancyButton = forwardRef(function(props, ref) { + if (props.fancy) { + useCustomHook(); + } + return ; + }); + `, + errors: [conditionalError('useCustomHook')], + }, { code: ` // Invalid because it's dangerous. diff --git a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js index 73636e60daf16..17f5655235b89 100644 --- a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js +++ b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js @@ -54,6 +54,28 @@ function isComponentName(node) { } } +function isForwardRef(node) { + return ( + node.name === 'forwardRef' || + (node.type === 'MemberExpression' && + node.object.name === 'React' && + node.property.name === 'forwardRef') + ); +} + +/** + * Checks if the node is a callback argument of forwardRef. This render function + * should follow the rules of hooks. + */ + +function isForwardRefCallback(node) { + return !!( + node.parent && + node.parent.callee && + isForwardRef(node.parent.callee) + ); +} + function isInsideComponentOrHook(node) { while (node) { const functionName = getFunctionName(node); @@ -62,6 +84,9 @@ function isInsideComponentOrHook(node) { return true; } } + if (isForwardRefCallback(node)) { + return true; + } node = node.parent; } return false; @@ -290,7 +315,8 @@ export default { // `undefined` then we know either that we have an anonymous function // expression or our code path is not in a function. In both cases we // will want to error since neither are React function components or - // hook functions. + // hook functions - unless it is an anonymous function argument to + // forwardRef. const codePathFunctionName = getFunctionName(codePathNode); // This is a valid code path for React hooks if we are directly in a React @@ -301,7 +327,7 @@ export default { const isDirectlyInsideComponentOrHook = codePathFunctionName ? isComponentName(codePathFunctionName) || isHook(codePathFunctionName) - : false; + : isForwardRefCallback(codePathNode); // Compute the earliest finalizer level using information from the // cache. We expect all reachable final segments to have a cache entry From e1cd0ad2f37791d6a7128f5f6a9e47d49c784ed1 Mon Sep 17 00:00:00 2001 From: David Garner Date: Sat, 2 Nov 2019 17:22:52 +0000 Subject: [PATCH 2/5] [eslint] Make tests more realistic (#17220) --- .../__tests__/ESLintRulesOfHooks-test.js | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js index be203be236cc0..9e26cd0f8a934 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js @@ -142,21 +142,27 @@ const tests = { } `, ` - // Valid because hooks can be used in forwardRef. - const FancyButton = React.forwardRef(() => { - return useHook(); + // Valid because hooks can be used in anonymous arrow-function arguments + // to forwardRef. + const FancyButton = React.forwardRef((props, ref) => { + useHook(); + return ; + }); + `, + errors: [conditionalError('useCustomHook')], + }, { code: ` // Invalid because it's dangerous. diff --git a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js index 17f5655235b89..dbcc9c3de59bb 100644 --- a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js +++ b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js @@ -54,12 +54,12 @@ function isComponentName(node) { } } -function isForwardRef(node) { +function isReactFunction(node, functionName) { return ( - node.name === 'forwardRef' || + node.name === functionName || (node.type === 'MemberExpression' && node.object.name === 'React' && - node.property.name === 'forwardRef') + node.property.name === functionName) ); } @@ -72,7 +72,20 @@ function isForwardRefCallback(node) { return !!( node.parent && node.parent.callee && - isForwardRef(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) { + return !!( + node.parent && + node.parent.callee && + isReactFunction(node.parent.callee, 'memo') ); } @@ -84,7 +97,7 @@ function isInsideComponentOrHook(node) { return true; } } - if (isForwardRefCallback(node)) { + if (isForwardRefCallback(node) || isMemoCallback(node)) { return true; } node = node.parent; @@ -316,7 +329,7 @@ export default { // expression or our code path is not in a function. In both cases we // will want to error since neither are React function components or // hook functions - unless it is an anonymous function argument to - // forwardRef. + // forwardRef or memo. const codePathFunctionName = getFunctionName(codePathNode); // This is a valid code path for React hooks if we are directly in a React @@ -327,7 +340,7 @@ export default { const isDirectlyInsideComponentOrHook = codePathFunctionName ? isComponentName(codePathFunctionName) || isHook(codePathFunctionName) - : isForwardRefCallback(codePathNode); + : isForwardRefCallback(codePathNode) || isMemoCallback(codePathNode); // Compute the earliest finalizer level using information from the // cache. We expect all reachable final segments to have a cache entry From 37e35543f02d3c8650302d73140f9d32fbb225f2 Mon Sep 17 00:00:00 2001 From: David Garner Date: Sat, 9 Nov 2019 17:27:29 +0000 Subject: [PATCH 4/5] [eslint] Add tests for callbacks not known to be components (#17220) --- .../__tests__/ESLintRulesOfHooks-test.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js index b4ba0b797bec2..8911100b63baf 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js @@ -302,6 +302,24 @@ const tests = { }); } `, + ` + // Similarly, this is valid because "use"-prefixed functions called in + // the callbacks of unrecognised functions are not assumed to be hooks. + React.unknownFunction((foo, bar) => { + if (foo) { + useNotAHook(bar) + } + }); + `, + ` + // Similarly, this is valid because "use"-prefixed functions called in + // the callbacks of unrecognised functions are not assumed to be hooks. + unknownFunction(function(foo, bar) { + if (foo) { + useNotAHook(bar) + } + }); + `, ` // Regression test for incorrectly flagged valid code. function RegressionTest() { From 54a180a5bbd7d7a8d7cbfe5fa98ed365ccbaa2c0 Mon Sep 17 00:00:00 2001 From: David Garner Date: Sat, 9 Nov 2019 17:59:02 +0000 Subject: [PATCH 5/5] [eslint] Correct comments and add another test (#17220) --- .../__tests__/ESLintRulesOfHooks-test.js | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js index 8911100b63baf..662e9cc39b963 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js @@ -303,8 +303,8 @@ const tests = { } `, ` - // Similarly, this is valid because "use"-prefixed functions called in - // the callbacks of unrecognised functions are not assumed to be hooks. + // This is valid because "use"-prefixed functions called in + // unnamed function arguments are not assumed to be hooks. React.unknownFunction((foo, bar) => { if (foo) { useNotAHook(bar) @@ -312,8 +312,8 @@ const tests = { }); `, ` - // Similarly, this is valid because "use"-prefixed functions called in - // the callbacks of unrecognised functions are not assumed to be hooks. + // This is valid because "use"-prefixed functions called in + // unnamed function arguments are not assumed to be hooks. unknownFunction(function(foo, bar) { if (foo) { useNotAHook(bar) @@ -818,6 +818,16 @@ const tests = { `, errors: [conditionalError('useCustomHook')], }, + { + code: ` + // This is invalid because "use"-prefixed functions used in named + // functions are assumed to be hooks. + React.unknownFunction(function notAComponent(foo, bar) { + useProbablyAHook(bar) + }); + `, + errors: [functionError('useProbablyAHook', 'notAComponent')], + }, { code: ` // Invalid because it's dangerous.