From d17deb8b6d05f650f9b7348dc80231ed9399d073 Mon Sep 17 00:00:00 2001 From: Rui Conti Date: Tue, 1 Feb 2022 21:54:56 -0300 Subject: [PATCH 1/9] Adds rule that verifies if anonymous function is properly used --- src/react-hooks-nesting-walker/error-messages.ts | 2 ++ .../react-hooks-nesting-walker.ts | 15 +++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/src/react-hooks-nesting-walker/error-messages.ts b/src/react-hooks-nesting-walker/error-messages.ts index 6a40580..d969bf1 100644 --- a/src/react-hooks-nesting-walker/error-messages.ts +++ b/src/react-hooks-nesting-walker/error-messages.ts @@ -17,4 +17,6 @@ export const ERROR_MESSAGES = { 'A hook cannot be used inside of another function', invalidFunctionExpression: 'A hook cannot be used inside of another function', hookAfterEarlyReturn: 'A hook should not appear after a return statement', + anonymousFunctionIllegalCallback: + 'Hook is in an anonymous function that is passed to an illegal callback', }; diff --git a/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts b/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts index f4eefd2..867f087 100644 --- a/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts +++ b/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts @@ -180,6 +180,21 @@ export class ReactHooksNestingWalker extends RuleWalker { return; } + /** + * Detect if the unnamed expression is wrapped in a illegal function call + */ + if ( + isCallExpression(ancestor.parent) && + isIdentifier(ancestor.parent.expression) && + !isReactComponentDecorator(ancestor.parent.expression) + ) { + this.addFailureAtNode( + hookNode, + ERROR_MESSAGES.anonymousFunctionIllegalCallback, + ); + return; + } + /** * Allow using hooks when the function is passed to `React.memo` or `React.forwardRef` */ From b75e46cb413b101af110bf3dd4d4223ea239eaf5 Mon Sep 17 00:00:00 2001 From: Rui Conti Date: Tue, 1 Feb 2022 21:55:21 -0300 Subject: [PATCH 2/9] Adds tiny set of tests --- .../anonymous-function-error.ts.lint | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 test/tslint-rule/anonymous-function-error.ts.lint diff --git a/test/tslint-rule/anonymous-function-error.ts.lint b/test/tslint-rule/anonymous-function-error.ts.lint new file mode 100644 index 0000000..b1bd9f1 --- /dev/null +++ b/test/tslint-rule/anonymous-function-error.ts.lint @@ -0,0 +1,19 @@ +const IllegalComponent = wrapper(function() { + useEffect(() => {}); + ~~~~~~~~~~~~~~~~~~~ [Hook is in an anonymous function that is passed to an illegal callback] +}) + +const LegalAnonymousComponent = function() { + useEffect(() => {}); +} + +const ForwardedComponent = React.forwardRef(function(props, ref) { + useEffect(() => { + console.log("I am legal") + }); +}) + +const MemoizedComponent = React.memo((props) => { + const [state, setState] = React.useState(props.initValue); + return {state} +}) \ No newline at end of file From e41cd6eba1da03b28f27856fcd480f51ca3dae81 Mon Sep 17 00:00:00 2001 From: Rui Conti Date: Wed, 2 Feb 2022 20:58:57 -0300 Subject: [PATCH 3/9] Simplifies and removes redundancy from rule's conditional --- .../react-hooks-nesting-walker.ts | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts b/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts index 867f087..c64ba61 100644 --- a/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts +++ b/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts @@ -181,27 +181,23 @@ export class ReactHooksNestingWalker extends RuleWalker { } /** - * Detect if the unnamed expression is wrapped in a illegal function call + * Allow using hooks when the function is passed to `React.memo` or `React.forwardRef` */ if ( isCallExpression(ancestor.parent) && - isIdentifier(ancestor.parent.expression) && - !isReactComponentDecorator(ancestor.parent.expression) + isReactComponentDecorator(ancestor.parent.expression) ) { - this.addFailureAtNode( - hookNode, - ERROR_MESSAGES.anonymousFunctionIllegalCallback, - ); return; } /** - * Allow using hooks when the function is passed to `React.memo` or `React.forwardRef` + * Detect if the unnamed expression is wrapped in a illegal function call */ - if ( - isCallExpression(ancestor.parent) && - isReactComponentDecorator(ancestor.parent.expression) - ) { + if (isIdentifier((ancestor.parent as CallExpression).expression)) { + this.addFailureAtNode( + hookNode, + ERROR_MESSAGES.anonymousFunctionIllegalCallback, + ); return; } From e8f1a5a35694f43b6f2547dcf77c60b1ed914f01 Mon Sep 17 00:00:00 2001 From: Rui Conti Date: Wed, 2 Feb 2022 20:59:12 -0300 Subject: [PATCH 4/9] Adds more information to the error message --- src/react-hooks-nesting-walker/error-messages.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/react-hooks-nesting-walker/error-messages.ts b/src/react-hooks-nesting-walker/error-messages.ts index d969bf1..918e0ac 100644 --- a/src/react-hooks-nesting-walker/error-messages.ts +++ b/src/react-hooks-nesting-walker/error-messages.ts @@ -17,6 +17,5 @@ export const ERROR_MESSAGES = { 'A hook cannot be used inside of another function', invalidFunctionExpression: 'A hook cannot be used inside of another function', hookAfterEarlyReturn: 'A hook should not appear after a return statement', - anonymousFunctionIllegalCallback: - 'Hook is in an anonymous function that is passed to an illegal callback', + anonymousFunctionIllegalCallback: `Hook is in an anonymous function that is passed to an illegal callback. Legal callbacks identifiers that can receive anonymous functions as arguments are memo and forwardRef`, }; From 9108ec24c3451113e09a71926fa8ba6d25b58791 Mon Sep 17 00:00:00 2001 From: Rui Conti Date: Wed, 2 Feb 2022 21:00:01 -0300 Subject: [PATCH 5/9] Adds a test scenario that disambigues the need for this specific rule --- test/tslint-rule/anonymous-function-error.ts.lint | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/test/tslint-rule/anonymous-function-error.ts.lint b/test/tslint-rule/anonymous-function-error.ts.lint index b1bd9f1..10e4d5e 100644 --- a/test/tslint-rule/anonymous-function-error.ts.lint +++ b/test/tslint-rule/anonymous-function-error.ts.lint @@ -1,6 +1,6 @@ const IllegalComponent = wrapper(function() { useEffect(() => {}); - ~~~~~~~~~~~~~~~~~~~ [Hook is in an anonymous function that is passed to an illegal callback] + ~~~~~~~~~~~~~~~~~~~ [Hook is in an anonymous function that is passed to an illegal callback. Legal callbacks identifiers that can receive anonymous functions as arguments are memo and forwardRef] }) const LegalAnonymousComponent = function() { @@ -16,4 +16,11 @@ const ForwardedComponent = React.forwardRef(function(props, ref) { const MemoizedComponent = React.memo((props) => { const [state, setState] = React.useState(props.initValue); return {state} -}) \ No newline at end of file +}) + +const Functor = function() { + const cb = React.useCallback(() => { + const r = React.useRef() + ~~~~~~~~~~~~~~ [A hook cannot be used inside of another function] + }, []) +} From 39cd396984d88777f474cb52cd68acb5e076e14e Mon Sep 17 00:00:00 2001 From: Rui Conti Date: Wed, 2 Feb 2022 21:03:10 -0300 Subject: [PATCH 6/9] Add missing conditional guard --- src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts b/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts index c64ba61..dc410df 100644 --- a/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts +++ b/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts @@ -193,7 +193,7 @@ export class ReactHooksNestingWalker extends RuleWalker { /** * Detect if the unnamed expression is wrapped in a illegal function call */ - if (isIdentifier((ancestor.parent as CallExpression).expression)) { + if (isIdentifier((ancestor.parent as CallExpression)?.expression)) { this.addFailureAtNode( hookNode, ERROR_MESSAGES.anonymousFunctionIllegalCallback, From b2d11aef7678a7842c8930c0fcb1a7f98428869a Mon Sep 17 00:00:00 2001 From: Rui Conti Date: Wed, 2 Feb 2022 22:55:37 -0300 Subject: [PATCH 7/9] Fixes deprecated ubuntu vmImage value https://github.com/actions/virtual-environments/issues/3287 --- azure-pipelines.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/azure-pipelines.yml b/azure-pipelines.yml index ef40948..b62eae2 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -1,5 +1,5 @@ pool: - vmImage: 'Ubuntu-16.04' + vmImage: 'ubuntu-18.04' steps: - task: NodeTool@0 From e8a4a70b1a46488957b330dd1ebecc63dc997de0 Mon Sep 17 00:00:00 2001 From: Rui Conti Date: Sat, 5 Feb 2022 12:10:38 -0300 Subject: [PATCH 8/9] Adds missing check for callback being a valid hook identifier --- .../react-hooks-nesting-walker.ts | 6 +++++- test/tslint-rule/anonymous-function-error.ts.lint | 5 +++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts b/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts index dc410df..4d852fe 100644 --- a/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts +++ b/src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts @@ -193,7 +193,11 @@ export class ReactHooksNestingWalker extends RuleWalker { /** * Detect if the unnamed expression is wrapped in a illegal function call */ - if (isIdentifier((ancestor.parent as CallExpression)?.expression)) { + if ( + isCallExpression(ancestor.parent) && + isIdentifier(ancestor.parent.expression) && + !isComponentOrHookIdentifier(ancestor.parent.expression) + ) { this.addFailureAtNode( hookNode, ERROR_MESSAGES.anonymousFunctionIllegalCallback, diff --git a/test/tslint-rule/anonymous-function-error.ts.lint b/test/tslint-rule/anonymous-function-error.ts.lint index 10e4d5e..6540ac8 100644 --- a/test/tslint-rule/anonymous-function-error.ts.lint +++ b/test/tslint-rule/anonymous-function-error.ts.lint @@ -23,4 +23,9 @@ const Functor = function() { const r = React.useRef() ~~~~~~~~~~~~~~ [A hook cannot be used inside of another function] }, []) + + const cb = useCallback(() => { + const r = React.useRef() + ~~~~~~~~~~~~~~ [A hook cannot be used inside of another function] + }, []) } From eca2ac74515de3a20688a1db32dea97c8c42f99b Mon Sep 17 00:00:00 2001 From: Rui Conti Date: Sun, 6 Feb 2022 22:43:53 -0300 Subject: [PATCH 9/9] Adds test scenario to cover for potential amibguous matching Hooks defined within anonymous functions which are not being passed to a callback. Which makes them automatically illegal. --- test/tslint-rule/anonymous-function-error.ts.lint | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/tslint-rule/anonymous-function-error.ts.lint b/test/tslint-rule/anonymous-function-error.ts.lint index 6540ac8..b0c37e9 100644 --- a/test/tslint-rule/anonymous-function-error.ts.lint +++ b/test/tslint-rule/anonymous-function-error.ts.lint @@ -28,4 +28,10 @@ const Functor = function() { const r = React.useRef() ~~~~~~~~~~~~~~ [A hook cannot be used inside of another function] }, []) + + obj[(props) => { useRef() }] = 123; + ~~~~~~~~ [A hook cannot be used inside of another function] + + new Abc((props) => { useRef() }) + ~~~~~~~~ [A hook cannot be used inside of another function] }