Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds illegal anonymous function error msg #30

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
+54 −1
Open

Adds illegal anonymous function error msg #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d17deb8
Adds rule that verifies if anonymous function is properly used
ruiconti Feb 2, 2022
b75e46c
Adds tiny set of tests
ruiconti Feb 2, 2022
e41cd6e
Simplifies and removes redundancy from rule's conditional
ruiconti Feb 2, 2022
e8f1a5a
Adds more information to the error message
ruiconti Feb 2, 2022
9108ec2
Adds a test scenario that disambigues the need for this specific rule
ruiconti Feb 3, 2022
39cd396
Add missing conditional guard
ruiconti Feb 3, 2022
b2d11ae
Fixes deprecated ubuntu vmImage value
ruiconti Feb 3, 2022
7e4048f
Merge branch 'master' into rconti/adds-illegal-annonymous-function-er…
ruiconti Feb 3, 2022
e8a4a70
Adds missing check for callback being a valid hook identifier
ruiconti Feb 5, 2022
eca2ac7
Adds test scenario to cover for potential amibguous matching
ruiconti Feb 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion azure-pipelines.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
pool:
vmImage: 'Ubuntu-16.04'
vmImage: 'ubuntu-18.04'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing that in #31! I'd appreciate if you rebased this PR on the latest master to deduplicate the commit that changes this value

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this change is still in "new" in this PR, but it was already merged to master. Could you rebase and drop the commits that are already on master?


steps:
- task: NodeTool@0
Expand Down
1 change: 1 addition & 0 deletions src/react-hooks-nesting-walker/error-messages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +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',
ruiconti marked this conversation as resolved.
Show resolved Hide resolved
hookAfterEarlyReturn: 'A hook should not appear after a return statement',
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`,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this error message, I am afraid it may be confusing for regular developers who were not working with ASTs/language parsing before.

Suggested change
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`,
anonymousFunctionIllegalCallback: `Hook is used in an anonymous function that is provided as an argument in an unexpected function. Functions that can receive anonymous functions with hook calls are "React.memo" and "React.forwardRef". If this is intended, add a name to the anonymous wrapping function to make it appear as a component.`,

};
15 changes: 15 additions & 0 deletions src/react-hooks-nesting-walker/react-hooks-nesting-walker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,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) &&
!isComponentOrHookIdentifier(ancestor.parent.expression)
Comment on lines +198 to +199
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mostly looks good now 👍 One change that I believe is worth it, is using isReactComponentDecorator here. This is because for the following snippet:

const IllegalComponent = Wrapper(function() {
  useEffect(() => {});
})

Wrapper is a component identifier, so the code does not report anonymousFunctionIllegalCallback, but it reports the more-generic invalidFunctionExpression error:

  const IllegalComponent = Wrapper(function() {
    useEffect(() => {});
    ~~~~~~~~~~~~~~~~~~~  [A hook cannot be used inside of another function]
  })

It should also work in this case, where the current code uses the more-generic message:

  const IllegalComponent = React.Wrapper(function() {
    useEffect(() => {});
    ~~~~~~~~~~~~~~~~~~~  [A hook cannot be used inside of another function]
  })

I would expect to see the anonymousFunctionIllegalCallback message in these cases.

When using

Suggested change
isIdentifier(ancestor.parent.expression) &&
!isComponentOrHookIdentifier(ancestor.parent.expression)
!isReactComponentDecorator(ancestor.parent.expression)

(note that I removed the isIdentifier check, because isReactComponentDecorator smartly detects it) we have the following (IMO correct) results in these 2 cases:

  const IllegalComponent = Wrapper(function() {
    useEffect(() => {});
    ~~~~~~~~~~~~~~~~~~~  [Hook is used in an anonymous function that is provided as an argument in an unexpected function. Functions that can receive anonymous functions with hook calls are "React.memo" and "React.forwardRef". If this is intended, add a name to the wrapping function to make it appear as a component.]
  })

  const IllegalComponent = React.Wrapper(function() {
    useEffect(() => {});
    ~~~~~~~~~~~~~~~~~~~  [Hook is used in an anonymous function that is provided as an argument in an unexpected function. Functions that can receive anonymous functions with hook calls are "React.memo" and "React.forwardRef". If this is intended, add a name to the wrapping function to make it appear as a component.]
  })

However, that also causes changes in other tests:

    // Usage inside other functions
    [].forEach(() => {
      useEffect(() => {});
-     ~~~~~~~~~~~~~~~~~~~  [A hook cannot be used inside of another function]
+     ~~~~~~~~~~~~~~~~~~~  [Hook is used in an anonymous function that is provided as an argument in an unexpected function. Functions that can receive anonymous functions with hook calls are "React.memo" and "React.forwardRef". If this is intended, add a name to the wrapping function to make it appear as a component.]
    })

I believe that is acceptable, but not sure if it's worth it - the new error message (either the one you suggested or the one from me) can mislead the engineer.

Overall, it seems like a hard problem to detect function calls and differentiate between situations when the user is using a hook in a component-like function, or a regular callback (e.g. array.forEach's callback) without an equivalent of isSomewhereInsideComponentOrHook.

I'm not sure how you want to proceed here. The current implementation adds this new type of error only in very special scenarios (only when the called function's name does not look like a hook/component, so uppercase function names are ignored) and does not detect all cases, even though the implementation suggests it does. We could modify the implementation to say that it detects some invalid function calls, or we could implement isSomewhereInsideComponentOrHook and make it more robust.

Personally, I would rather implement a more robust solution to try to make the error messages more precise

) {
this.addFailureAtNode(
hookNode,
ERROR_MESSAGES.anonymousFunctionIllegalCallback,
);
return;
}

// Disallow using hooks inside other kinds of functions
this.addFailureAtNode(hookNode, ERROR_MESSAGES.invalidFunctionExpression);
return;
Expand Down
37 changes: 37 additions & 0 deletions test/tslint-rule/anonymous-function-error.ts.lint
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
const IllegalComponent = wrapper(function() {
useEffect(() => {});
~~~~~~~~~~~~~~~~~~~ [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() {
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 <span>{state}</span>
})

const Functor = function() {
const cb = React.useCallback(() => {
Gelio marked this conversation as resolved.
Show resolved Hide resolved
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]
}, [])

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]
}