Adds illegal anonymous function error msg

[ruiconti]

The only legal scenario for a React component function to be anonymous i.e. unnamed, is when it is passed as a callback to either React.forwardRef or React.memo.

This can be verified on eslint's RulesOfHooks. It uses a different eliminatory logic to rule out violations than this project, so we won't have a 1:1 match in terms of rule verification. But essentially, if a hook's ancestor (AST) node —the function declaration statement— is not in a forwardRef or memo call AND has no valid identifier (conditional stated here), than it falls back to this branch.

Current behavior would treat this error as "A hook hook cannot be used inside of another function", which is misleading.

Resolves #29.

[Gelio]

Thank you for your contribution! I gave it a quick read and it looks to be working well 👍 I appreciate the thorough PR description, links to exact code from the ESLint rule help a lot

I left some comments and questions to broaden my understanding and hopefully make it even better.

I will have time probably next week to give it a deeper look and also fix the Azure Pipelines, which seem not to be working

[Gelio]

Funny thing, today is the 3rd anniversary of the release of this TSLint rule 😄

Thanks for making further changes! I'm afraid I found a couple of scenarios that result in incorrect error messages being returned 😟 Take a look at the comments

[Gelio]

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

[Gelio]

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?

[ruiconti]

Hey, thanks for the thorough review!

As for the changes of this PR, I believe that ensuring that the ancestor's parent node is not a hook or a component clears out the scenarios you mentioned that were being misclassified. I can't think of any more of them, but I appreciate if you can exhaust this further.

---

After reviewing your comments, I realized that you are correct: the walker is missing being aware of the broader tree path it is in, and whether it is inside a nested valid component or hook. Which seems to be another issue altogether.

Consider the following statement:

const a = forwardRef(() => {
  while(true) {
    return React.memo(() => {
      const x = useRef();
    })
  }
})

It is an invalid usage of hooks but it doesn't trigger any rules, because current rules rely only on, at most, two levels above ancestor.parent in order to trigger some rules.

Edit: it is a valid snippet for this tslint rule.

[Gelio]

Thanks for introducing more updates 👍

It is an invalid usage of hooks but it doesn't trigger any rules, because current rules rely only on, at most, two levels above ancestor.parent in order to trigger some rules.

In terms of this rule, it does not seem that invalid to me. In the snippet you shared, useRef is called within React.memo, which is a valid context for a hook call. The fact that we're returning a component type instead of a component instance from forwardRef is beyond the responsibility of this rule (and a linter in general, IMO). If you asked me, this is the type of error that should be caught by TypeScript because it's a type error and not strictly related to hooks.

[ruiconti]

In the snippet you shared, useRef is called within React.memo, which is a valid context for a hook call.

That is correct. I was attempting to exercise corner cases that relied on a broader context awareness, but it's not the case for this particular linter —which relies mostly on the enclosing scope in which the hook is defined. Appreciate your patience on clearing those out 👍

Please let me know if you think there's anything left prior to sign off.

[Gelio]

Sorry for the long wait, I have bit a bit busy last week. Also, I have just noticed some of my comments were not published during my last review. I'm posting them now

I will have time to review more changes next weekend.

[Gelio]

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.`,

[Gelio]

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

[Gelio]

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?