From c89a83695c18814e98a8e815a2389a11540e0160 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Fri, 23 Sep 2022 14:55:12 -0700 Subject: [PATCH] Update RulesOfHooks with useEvent rules (#25285) This update to the RulesOfHooks rule checks that functions created with `useEvent` can only be invoked in a `useEffect` callback, in another event function, or a closure. They can't be passed down directly as a reference to child components. This PR also updates the ExhaustiveDeps lint rule to treat useEvent's return value as stable, so it can be omitted from dependency lists. Currently this all gated behind an experimental flag. Co-authored-by: Dan Abramov --- .../ESLintRuleExhaustiveDeps-test.js | 20 ++- .../__tests__/ESLintRulesOfHooks-test.js | 158 +++++++++++++++++- .../src/ExhaustiveDeps.js | 12 ++ .../src/RulesOfHooks.js | 102 +++++++++++ 4 files changed, 290 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index 5c3c64c173fa9..2f9764a74db19 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -1630,7 +1630,7 @@ const tests = { }, 1000); return () => clearInterval(id); }, [setCount]); - + return

{count}

; } `, @@ -7630,6 +7630,24 @@ const tests = { ], }; +if (__EXPERIMENTAL__) { + tests.valid = [ + ...tests.valid, + { + code: normalizeIndent` + function MyComponent({ theme }) { + const onStuff = useEvent(() => { + showNotification(theme); + }); + useEffect(() => { + onStuff(); + }, []); + } + `, + }, + ]; +} + // Tests that are only valid/invalid across parsers supporting Flow const testsFlow = { valid: [ diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js index ee220fa5dba3c..16bec9eb588a9 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js @@ -449,7 +449,7 @@ const tests = { }, { code: ` - // This is a false positive (it's valid) that unfortunately + // This is a false positive (it's valid) that unfortunately // we cannot avoid. Prefer to rename it to not start with "use" class Foo extends Component { render() { @@ -974,6 +974,154 @@ const tests = { ], }; +if (__EXPERIMENTAL__) { + tests.valid = [ + ...tests.valid, + ` + // Valid because functions created with useEvent can be called in a useEffect. + function MyComponent({ theme }) { + const onClick = useEvent(() => { + showNotification(theme); + }); + useEffect(() => { + onClick(); + }); + } + `, + ` + // Valid because functions created with useEvent can be called in closures. + function MyComponent({ theme }) { + const onClick = useEvent(() => { + showNotification(theme); + }); + return onClick()}>; + } + `, + ` + // Valid because functions created with useEvent can be called in closures. + function MyComponent({ theme }) { + const onClick = useEvent(() => { + showNotification(theme); + }); + const onClick2 = () => { onClick() }; + const onClick3 = useCallback(() => onClick(), []); + return <> + + + ; + } + `, + ` + // Valid because functions created with useEvent can be passed by reference in useEffect + // and useEvent. + function MyComponent({ theme }) { + const onClick = useEvent(() => { + showNotification(theme); + }); + const onClick2 = useEvent(() => { + debounce(onClick); + }); + useEffect(() => { + let id = setInterval(onClick, 100); + return () => clearInterval(onClick); + }, []); + return onClick2()} /> + } + `, + ` + const MyComponent = ({theme}) => { + const onClick = useEvent(() => { + showNotification(theme); + }); + return onClick()}>; + }; + `, + ` + function MyComponent({ theme }) { + const notificationService = useNotifications(); + const showNotification = useEvent((text) => { + notificationService.notify(theme, text); + }); + const onClick = useEvent((text) => { + showNotification(text); + }); + return onClick(text)} /> + } + `, + ` + function MyComponent({ theme }) { + useEffect(() => { + onClick(); + }); + const onClick = useEvent(() => { + showNotification(theme); + }); + } + `, + ]; + tests.invalid = [ + ...tests.invalid, + { + code: ` + function MyComponent({ theme }) { + const onClick = useEvent(() => { + showNotification(theme); + }); + return ; + } + `, + errors: [useEventError('onClick')], + }, + { + code: ` + // This should error even though it shares an identifier name with the below + function MyComponent({theme}) { + const onClick = useEvent(() => { + showNotification(theme) + }); + return + } + + // The useEvent function shares an identifier name with the above + function MyOtherComponent({theme}) { + const onClick = useEvent(() => { + showNotification(theme) + }); + return onClick()} /> + } + `, + errors: [{...useEventError('onClick'), line: 4}], + }, + { + code: ` + const MyComponent = ({ theme }) => { + const onClick = useEvent(() => { + showNotification(theme); + }); + return ; + } + `, + errors: [useEventError('onClick')], + }, + { + code: ` + // Invalid because onClick is being aliased to foo but not invoked + function MyComponent({ theme }) { + const onClick = useEvent(() => { + showNotification(theme); + }); + let foo; + useEffect(() => { + foo = onClick; + }); + return + } + `, + errors: [useEventError('onClick')], + }, + ]; +} + function conditionalError(hook, hasPreviousFinalizer = false) { return { message: @@ -1031,6 +1179,14 @@ function classError(hook) { }; } +function useEventError(fn) { + return { + message: + `\`${fn}\` is a function created with React Hook "useEvent", and can only be called from ` + + 'the same component. They cannot be assigned to variables or passed down.', + }; +} + // For easier local testing if (!process.env.CI) { let only = []; diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index c2341886872c1..2ebaf21c62c30 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -157,6 +157,8 @@ export default { // ^^^ true for this reference // const ref = useRef() // ^^^ true for this reference + // const onStuff = useEvent(() => {}) + // ^^^ true for this reference // False for everything else. function isStableKnownHookValue(resolved) { if (!isArray(resolved.defs)) { @@ -223,6 +225,9 @@ export default { if (name === 'useRef' && id.type === 'Identifier') { // useRef() return value is stable. return true; + } else if (isUseEventIdentifier(callee) && id.type === 'Identifier') { + // useEvent() return value is stable. + return true; } else if (name === 'useState' || name === 'useReducer') { // Only consider second value in initializing tuple stable. if ( @@ -1819,3 +1824,10 @@ function isSameIdentifier(a, b) { function isAncestorNodeOf(a, b) { return a.range[0] <= b.range[0] && a.range[1] >= b.range[1]; } + +function isUseEventIdentifier(node) { + if (__EXPERIMENTAL__) { + return node.type === 'Identifier' && node.name === 'useEvent'; + } + return false; +} diff --git a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js index 1957d3ce199af..20ceb24244a9e 100644 --- a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js +++ b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js @@ -100,6 +100,13 @@ function isInsideComponentOrHook(node) { return false; } +function isUseEventIdentifier(node) { + if (__EXPERIMENTAL__) { + return node.type === 'Identifier' && node.name === 'useEvent'; + } + return false; +} + export default { meta: { type: 'problem', @@ -110,8 +117,45 @@ export default { }, }, create(context) { + let lastEffect = null; const codePathReactHooksMapStack = []; const codePathSegmentStack = []; + const useEventViolations = new Set(); + + // For a given AST node, iterate through the top level statements and add all useEvent + // definitions. We can do this in non-Program nodes because we can rely on the assumption that + // useEvent functions can only be declared within a component or hook at its top level. + function addAllUseEventViolations(node) { + if (node.body.type !== 'BlockStatement') return; + for (const statement of node.body.body) { + if (statement.type !== 'VariableDeclaration') continue; + for (const declaration of statement.declarations) { + if ( + declaration.type === 'VariableDeclarator' && + declaration.init && + declaration.init.type === 'CallExpression' && + declaration.init.callee && + isUseEventIdentifier(declaration.init.callee) + ) { + useEventViolations.add(declaration.id); + } + } + } + } + + // Resolve a useEvent violation, ie the useEvent created function was called. + function resolveUseEventViolation(scope, ident) { + if (scope.references == null || useEventViolations.size === 0) return; + for (const ref of scope.references) { + if (ref.resolved == null) continue; + const [useEventFunctionIdentifier] = ref.resolved.identifiers; + if (ident.name === useEventFunctionIdentifier.name) { + useEventViolations.delete(useEventFunctionIdentifier); + break; + } + } + } + return { // Maintain code segment path stack as we traverse. onCodePathSegmentStart: segment => codePathSegmentStack.push(segment), @@ -522,6 +566,64 @@ export default { } reactHooks.push(node.callee); } + + const scope = context.getScope(); + // useEvent: Resolve a function created with useEvent that is invoked locally at least once. + // OK - onClick(); + resolveUseEventViolation(scope, node.callee); + + // useEvent: useEvent functions can be passed by reference within useEffect as well as in + // another useEvent + if ( + node.callee.type === 'Identifier' && + (node.callee.name === 'useEffect' || + isUseEventIdentifier(node.callee)) && + node.arguments.length > 0 + ) { + // Denote that we have traversed into a useEffect call, and stash the CallExpr for + // comparison later when we exit + lastEffect = node; + } + }, + + Identifier(node) { + // OK - useEffect(() => { setInterval(onClick, ...) }, []); + if (lastEffect != null && node.parent.type === 'CallExpression') { + resolveUseEventViolation(context.getScope(), node); + } + }, + + 'CallExpression:exit'(node) { + if (node === lastEffect) { + lastEffect = null; + } + }, + + FunctionDeclaration(node) { + // function MyComponent() { const onClick = useEvent(...) } + if (isInsideComponentOrHook(node)) { + addAllUseEventViolations(node); + } + }, + + ArrowFunctionExpression(node) { + // const MyComponent = () => { const onClick = useEvent(...) } + if (isInsideComponentOrHook(node)) { + addAllUseEventViolations(node); + } + }, + + 'Program:exit'(_node) { + for (const node of useEventViolations.values()) { + context.report({ + node, + message: + `\`${context.getSource( + node, + )}\` is a function created with React Hook "useEvent", and can only be called from ` + + 'the same component. They cannot be assigned to variables or passed down.', + }); + } }, }; },